From eafb9853fae6eb285623acc96c3f3ac8d68f8e31 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:03:57 -0400 Subject: [PATCH 01/35] OpenConceptLab/ocl_online#224 | Concept lookup: check view access and cache only public sources Restores e1f31381 (reverted in 7bfb6c90). The lookup view accepts any source, not only the public OCL lookup repos, so it needs the view check; and its page cache is keyed on the URL, so only public sources are cached. Co-Authored-By: Claude Opus 5.5 --- core/concepts/urls.py | 5 +-- core/concepts/views.py | 26 +++++++++++++-- .../tests_public_read_access.py | 33 +++++++++++++++++++ 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/core/concepts/urls.py b/core/concepts/urls.py index 1a0004a9f..ae9c6f59f 100644 --- a/core/concepts/urls.py +++ b/core/concepts/urls.py @@ -1,5 +1,4 @@ from django.urls import path -from django.views.decorators.cache import cache_page from core.concepts.feeds import ConceptFeed from . import views @@ -8,9 +7,7 @@ path('', views.ConceptListView.as_view(), name='concept-list'), path( 'lookup/', - cache_page( - timeout=60 * 60 * 24, key_prefix='cache_lookup' - )(views.ConceptLookupValuesView.as_view()), + views.cache_public_source_lookup(views.ConceptLookupValuesView.as_view()), name='concept-lookup-list' ), path( diff --git a/core/concepts/views.py b/core/concepts/views.py index 711c22ddd..5021f41c8 100644 --- a/core/concepts/views.py +++ b/core/concepts/views.py @@ -6,6 +6,7 @@ from django.db import transaction from django.db.models import F, Q as DjangoQ from django.http import Http404 +from django.views.decorators.cache import cache_page from drf_yasg import openapi from drf_yasg.utils import swagger_auto_schema from elasticsearch_dsl import Q @@ -104,7 +105,28 @@ def set_parent_resource(self, __pop=True): self.kwargs['parent_resource'] = self.parent_resource = parent_resource -# this is a cached view (expiry 24 hours) +def cache_public_source_lookup(view_func): + """Caches lookups of public sources only: a cached response is served to anyone, + without the view's access check.""" + cached_view_func = cache_page(timeout=60 * 60 * 24, key_prefix='cache_lookup')(view_func) + + def wrapper(request, *args, **kwargs): + from core.sources.models import Source + owner_filters = None + if 'org' in kwargs: + owner_filters = {'organization__mnemonic': kwargs['org']} + elif 'user' in kwargs: + owner_filters = {'user__username': kwargs['user']} + source = Source.get_version(kwargs['source'], HEAD, owner_filters) if owner_filters and kwargs.get( + 'source') else None + if source and source.public_can_view: + return cached_view_func(request, *args, **kwargs) + return view_func(request, *args, **kwargs) + + return wrapper + + +# this is a cached view for public sources (expiry 24 hours) # used for TermBrowser forms lookup values -- map-types/locales/datatypes/etc class ConceptLookupValuesView(ListAPIView, BaseAPIView): # pragma: no cover serializer_class = ConceptLookupListSerializer @@ -125,7 +147,7 @@ def set_parent_resource(self): def get_queryset(self): self.set_parent_resource() - if self.parent_resource: + if self.parent_resource and self.parent_resource.has_view_access(self.request.user): queryset = self.parent_resource.concepts_set.filter(id=F('versioned_object_id')) if self.is_verbose(): queryset = queryset.prefetch_related('names') diff --git a/core/integration_tests/tests_public_read_access.py b/core/integration_tests/tests_public_read_access.py index 52a2b2fe9..c0dfa8c61 100644 --- a/core/integration_tests/tests_public_read_access.py +++ b/core/integration_tests/tests_public_read_access.py @@ -1,3 +1,6 @@ +from django.core.cache import cache +from django.test import override_settings + from core.collections.tests.factories import OrganizationCollectionFactory from core.common.constants import ACCESS_TYPE_NONE from core.common.feeds import FeedFilterMixin, MAX_LIMIT, DEFAULT_LIMIT @@ -24,6 +27,36 @@ def setUp(self): self.public_concept = ConceptFactory(parent=self.public_source, mnemonic='shown') +@override_settings(CACHES={'default': {'BACKEND': 'django.core.cache.backends.locmem.LocMemCache'}}) +class ConceptLookupAccessTest(PrivateRepoReadAccessBaseTest): + def setUp(self): + super().setUp() + cache.clear() + + def lookup(self, source, user=None): + headers = {'HTTP_AUTHORIZATION': 'Token ' + user.get_token()} if user else {} + return self.client.get(source.uri + 'concepts/lookup/', **headers) + + def test_private_source_lookup_only_for_members(self): + response = self.lookup(self.private_source, self.member) + + self.assertEqual(response.status_code, 200) + self.assertEqual([concept['id'] for concept in response.data], ['hidden']) + + # a member's response must not be served to others from the cache + for user in [None, self.outsider]: + response = self.lookup(self.private_source, user) + + self.assertEqual(response.status_code, 404) + + def test_public_source_lookup_for_anyone(self): + for user in [None, self.outsider, self.member]: + response = self.lookup(self.public_source, user) + + self.assertEqual(response.status_code, 200) + self.assertEqual([concept['id'] for concept in response.data], ['shown']) + + class FeedAccessTest(PrivateRepoReadAccessBaseTest): def test_source_feed_only_for_public_sources(self): self.assertEqual(self.client.get(self.private_source.uri + 'atom/').status_code, 404) From 0a69bdd015656c2a04da6ed0cf974b93f06cf72c Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:11:28 -0400 Subject: [PATCH 02/35] OpenConceptLab/ocl_online#224 | $match: filter on exact-case owner and source names `owner` and `source` in the concepts index are lowercase-normalized, but owner names that differ only in case belong to different users or orgs. The visible-repo filter and the target repo filter now use the exact-case owner_text.keyword and source_text.keyword fields, and a `target_repo` is searched by the stored names of the repos that were checked, so a `version` key or a lowercase `owner_type` now works too. Co-Authored-By: Claude Opus 5.5 --- core/common/search.py | 14 +++-- core/concepts/search.py | 9 +++- core/concepts/views.py | 16 ++++-- .../tests_search_resolution_access.py | 53 ++++++++++++++++--- 4 files changed, 76 insertions(+), 16 deletions(-) diff --git a/core/common/search.py b/core/common/search.py index 65788f133..3497d889b 100644 --- a/core/common/search.py +++ b/core/common/search.py @@ -17,17 +17,23 @@ from core.common.utils import is_url_encoded_string +# Exact-case owner field: `owner` is lowercase-normalized, and owner names that differ only in case belong to +# different users or orgs. +EXACT_OWNER_FIELD = 'owner_text.keyword' + + def get_visible_repo_criteria(user): - """ES filter for concepts/mappings in repos the user can view: public ones, their own and their orgs'. + """ES filter for concepts in repos the user can view: public ones, their own and their orgs'. None for staff, who can view everything.""" if get(user, 'is_staff'): return None should = [Q('term', public_can_view=True)] if get(user, 'is_authenticated'): - should.append(Q('bool', must=[Q('term', owner_type='User'), Q('term', owner=user.username.lower())])) - org_mnemonics = [mnemonic.lower() for mnemonic in user.organizations.values_list('mnemonic', flat=True)] + should.append(Q('bool', must=[Q('term', owner_type='User'), Q('term', **{EXACT_OWNER_FIELD: user.username})])) + org_mnemonics = list(user.organizations.values_list('mnemonic', flat=True)) if org_mnemonics: - should.append(Q('bool', must=[Q('term', owner_type='Organization'), Q('terms', owner=org_mnemonics)])) + should.append(Q('bool', must=[ + Q('term', owner_type='Organization'), Q('terms', **{EXACT_OWNER_FIELD: org_mnemonics})])) return Q('bool', should=should, minimum_should_match=1) diff --git a/core/concepts/search.py b/core/concepts/search.py index fc7bb2bdb..8c7c03b15 100644 --- a/core/concepts/search.py +++ b/core/concepts/search.py @@ -3,7 +3,7 @@ from core.common.constants import FACET_SIZE, HEAD from core.common.lexical_variants import LexicalVariantDictionary -from core.common.search import CustomESFacetedSearch, CustomESSearch +from core.common.search import CustomESFacetedSearch, CustomESSearch, EXACT_OWNER_FIELD from core.common.utils import get_embeddings, is_canonical_uri from core.concepts.models import Concept @@ -124,6 +124,8 @@ class ConceptFuzzySearch: # pragma: no cover ['concept_class', 'datatype', 0], ] fuzzy_fields = ['name', 'synonyms'] + # `owner` and `source` are lowercase-normalized; names that differ only in case are different repos + exact_repo_fields = {'owner': EXACT_OWNER_FIELD, 'source': 'source_text.keyword'} @staticmethod def get_target_repo(repo_url): @@ -436,7 +438,10 @@ def get_filter_criteria(cls, data, include_retired, repo_params, repo_url, addit must_clauses = [] repo_params = repo_params or cls.get_target_repo_params(repo_url) for field, value in repo_params.items(): - must_clauses.append(Q('match', **{field: value})) + if field in cls.exact_repo_fields: + must_clauses.append(Q('term', **{cls.exact_repo_fields[field]: value})) + else: + must_clauses.append(Q('match', **{field: value})) if not include_retired: must_clauses.append(Q('match', retired=False)) for field in cls.filter_fields: diff --git a/core/concepts/views.py b/core/concepts/views.py index 5021f41c8..1f689aba2 100644 --- a/core/concepts/views.py +++ b/core/concepts/views.py @@ -1173,11 +1173,19 @@ def get_repo_params(is_semantic, target_repo_params, target_repo_url, user=None) raise Http400(f'Unable to resolve "target_repo_url": "{target_repo_url}"') repo_params = ConceptFuzzySearch.get_repo_params(repo) elif target_repo_params: - repos = MetadataToConceptsListView.get_target_repos_from_params(target_repo_params) - if not repos.exists() or not all(target_repo.has_view_access(user) for target_repo in repos): + repos = list(MetadataToConceptsListView.get_target_repos_from_params(target_repo_params)) + if not repos or not all(target_repo.has_view_access(user) for target_repo in repos): raise Http400(f'Unable to resolve "target_repo": "{target_repo_params}"') - repo = repos.first() - repo_params = target_repo_params + repo = repos[0] + # search the repos that were checked, by their stored names: target_repo's own values may differ + # in case or key names (e.g. "version", "org"), which the search filters would read differently + repo_params = {'owner': repo.parent.mnemonic, 'source': repo.mnemonic} + owner_types = {target_repo.parent.resource_type for target_repo in repos} + if len(owner_types) == 1: + repo_params['owner_type'] = owner_types.pop() + version = get(target_repo_params, 'source_version') or get(target_repo_params, 'version') + if version: + repo_params['source_version'] = version else: raise Http400(f'Unable to resolve "target_repo_url": "{target_repo_url}"') if is_semantic and not repo.has_semantic_match_algorithm: diff --git a/core/integration_tests/tests_search_resolution_access.py b/core/integration_tests/tests_search_resolution_access.py index af6aeb963..45016338e 100644 --- a/core/integration_tests/tests_search_resolution_access.py +++ b/core/integration_tests/tests_search_resolution_access.py @@ -4,7 +4,10 @@ from core.common.constants import ACCESS_TYPE_NONE from core.common.search import get_visible_repo_criteria from core.common.tests import OCLAPITestCase, PREVIEW_GROUP_NAME +from core.concepts.documents import ConceptDocument +from core.concepts.models import Concept from core.concepts.search import ConceptFuzzySearch +from core.concepts.tests.factories import ConceptFactory from core.orgs.tests.factories import OrganizationFactory from core.sources.tests.factories import OrganizationSourceFactory from core.url_registry.factories import GlobalURLRegistryFactory @@ -35,9 +38,11 @@ def test_criteria(self): member = get_visible_repo_criteria(self.member).to_dict() self.assertEqual(member['bool']['minimum_should_match'], 1) self.assertIn({'bool': {'must': [ - {'term': {'owner_type': 'User'}}, {'term': {'owner': 'resolvemember'}}]}}, member['bool']['should']) + {'term': {'owner_type': 'User'}}, {'term': {'owner_text.keyword': 'resolvemember'}}]}}, + member['bool']['should']) self.assertIn({'bool': {'must': [ - {'term': {'owner_type': 'Organization'}}, {'terms': {'owner': ['resolveorg']}}]}}, member['bool']['should']) + {'term': {'owner_type': 'Organization'}}, {'terms': {'owner_text.keyword': ['ResolveOrg']}}]}}, + member['bool']['should']) outsider = get_visible_repo_criteria(self.outsider).to_dict() self.assertEqual(len(outsider['bool']['should']), 2) @@ -65,14 +70,13 @@ def test_match_search_is_limited_to_visible_repos(self, get_repo_params_mock): additional_filter_criterion = search_mock.call_args[0][8] self.assertEqual(additional_filter_criterion, get_visible_repo_criteria(self.outsider)) - def match(self, user, target_repo_url, target_repo=None): - data = {'rows': [{'name': 'foo'}]} + def match(self, user, target_repo_url, target_repo=None, row=None, url='/concepts/$match/'): # pylint: disable=too-many-arguments + data = {'rows': [row or {'name': 'foo'}]} if target_repo_url: data['target_repo_url'] = target_repo_url if target_repo: data['target_repo'] = target_repo - return self.client.post( - '/concepts/$match/', data, HTTP_AUTHORIZATION='Token ' + user.get_token(), format='json') + return self.client.post(url, data, HTTP_AUTHORIZATION='Token ' + user.get_token(), format='json') def test_match_target_repo_needs_view_access(self): preview = Group.objects.get(name=PREVIEW_GROUP_NAME) @@ -102,6 +106,43 @@ def test_match_target_repo_needs_view_access(self): self.match(user, target_repo_url, target_repo) self.assertEqual(search_mock.call_args[0][2], private_target_repo) + def test_match_target_repo_is_searched_by_its_stored_names(self): + self.member.groups.add(Group.objects.get(name=PREVIEW_GROUP_NAME)) + + with patch.object(ConceptFuzzySearch, 'search', side_effect=StopSearch) as search_mock: + with self.assertRaises(StopSearch): + self.match( + self.member, None, + {'owner': 'ResolveOrg', 'owner_type': 'org', 'source': 'ResolveSource', 'version': 'HEAD'}) + self.assertEqual( + search_mock.call_args[0][2], + {'owner': 'ResolveOrg', 'owner_type': 'Organization', 'source': 'ResolveSource', 'source_version': 'HEAD'}) + + def test_match_tells_apart_owners_whose_names_differ_only_in_case(self): + self.patch_concept_es_mapping_for_ci() + self.outsider.groups.add(Group.objects.get(name=PREVIEW_GROUP_NAME)) + own_org = OrganizationFactory(mnemonic='resolveorg', public_access=ACCESS_TYPE_NONE) + own_org.members.add(self.outsider) + own_source = OrganizationSourceFactory( + organization=own_org, mnemonic='ResolveSource', public_access=ACCESS_TYPE_NONE) + for source in [own_source, self.private_source]: + concept = ConceptFactory(parent=source, mnemonic='case-collision', public_access=ACCESS_TYPE_NONE) + source.concepts.add(concept.get_latest_version()) + ConceptDocument().update(Concept.objects.filter(mnemonic='case-collision'), refresh=True) + + for target_repo_url, target_repo in [ + (own_source.uri, None), + (None, {'owner': 'resolveorg', 'owner_type': 'Organization', 'source': 'ResolveSource'})]: + response = self.match( + self.outsider, target_repo_url, target_repo, row={'id': 'case-collision'}, + url='/concepts/$match/?limit=10') + + self.assertEqual(response.status_code, 200) + self.assertEqual( + {result['url'] for result in response.data[0]['results']}, + {'/orgs/resolveorg/sources/ResolveSource/concepts/case-collision/'}, + (target_repo_url, target_repo)) + def test_match_head_target_repo_with_bare_url_searches_head(self): # the HEAD version_url is the bare repo URL, which resolves to the latest released version OrganizationSourceFactory( From 5fe85437ba275fd2fe7ae650f4a10f5eff4c4808 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:15:55 -0400 Subject: [PATCH 03/35] OpenConceptLab/ocl_online#224 | /parents/ and /children/ list only concepts the requester can view The same filter the includeParentConcepts/includeChildConcepts fields use (get_viewable_concepts), so a concept's parents or children in another repo show only when the requester can view that repo. Co-Authored-By: Claude Opus 5.5 --- core/concepts/views.py | 7 +++--- .../tests_cross_repo_access.py | 25 +++++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/core/concepts/views.py b/core/concepts/views.py index 1f689aba2..5ef49744c 100644 --- a/core/concepts/views.py +++ b/core/concepts/views.py @@ -61,7 +61,8 @@ ConceptDetailSerializer, ConceptListSerializer, ConceptDescriptionSerializer, ConceptNameSerializer, ConceptVersionDetailSerializer, ConceptVersionListSerializer, ConceptSummarySerializer, ConceptMinimalSerializer, - ConceptChildrenSerializer, ConceptParentsSerializer, ConceptLookupListSerializer, ConceptChecksumSerializer) + ConceptChildrenSerializer, ConceptParentsSerializer, ConceptLookupListSerializer, ConceptChecksumSerializer, + get_viewable_concepts) from core.mappings.serializers import MappingListSerializer from core.sources.clone_limits import clone_limit_error_detail, clone_lock, get_clone_budget from core.sources.models import CloneError, CloneLimitExceeded @@ -605,7 +606,7 @@ def get_queryset(self): raise Http404() self.check_object_permissions(self.request, instance) - return instance.child_concept_queryset() + return get_viewable_concepts(instance.child_concept_queryset(), self.request) def get(self, request, *args, **kwargs): return self.list(request, *args, **kwargs) @@ -623,7 +624,7 @@ def get_queryset(self): raise Http404() self.check_object_permissions(self.request, instance) - return instance.parent_concept_queryset() + return get_viewable_concepts(instance.parent_concept_queryset(), self.request) def get(self, request, *args, **kwargs): return self.list(request, *args, **kwargs) diff --git a/core/integration_tests/tests_cross_repo_access.py b/core/integration_tests/tests_cross_repo_access.py index bdb763a77..35e037aa2 100644 --- a/core/integration_tests/tests_cross_repo_access.py +++ b/core/integration_tests/tests_cross_repo_access.py @@ -169,6 +169,31 @@ def test_include_parent_concepts_hides_private_parents(self): self.assertEqual( sorted(concept['id'] for concept in response.data['parent_concepts']), ['public', 'secret']) + def test_parents_and_children_lists_hide_private_concepts(self): + child = self.persist_child('listedchild', [self.private_concept.uri, self.public_concept.uri]) + private_child = Concept.persist_new({ + **factory.build(dict, FACTORY_CLASS=ConceptFactory), 'mnemonic': 'privchild', + 'parent': self.private_source, 'public_access': ACCESS_TYPE_NONE, + 'names': [ConceptNameFactory.build(locale='en', locale_preferred=True)], + 'parent_concept_urls': [self.public_concept.uri] + }, self.member) + self.assertEqual(private_child.errors, {}) + + for user, parents, children in [ + (None, ['public'], ['listedchild']), + (self.outsider, ['public'], ['listedchild']), + (self.member, ['public', 'secret'], ['listedchild', 'privchild']), + (self.admin, ['public', 'secret'], ['listedchild', 'privchild'])]: + headers = {'HTTP_AUTHORIZATION': 'Token ' + user.get_token()} if user else {} + + response = self.client.get(child.uri + 'parents/', **headers) + self.assertEqual(response.status_code, 200) + self.assertEqual(sorted(concept['id'] for concept in response.data), parents, user) + + response = self.client.get(self.public_concept.uri + 'children/', **headers) + self.assertEqual(response.status_code, 200) + self.assertEqual(sorted(concept['id'] for concept in response.data), children, user) + class MappingTargetAccessTest(CrossRepoAccessBaseTest): def test_outsider_mapping_to_private_concept_is_external(self): From 9c906ebbe603d0efab7e100547bbd58472f0ac49 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:18:36 -0400 Subject: [PATCH 04/35] OpenConceptLab/ocl_online#224 | Don't cache repo version concept lists that include parent or child concepts includeParentConcepts and includeChildConcepts show only concepts the requester can view, so these responses differ per requester and can't come from, or go into, the shared list cache. Co-Authored-By: Claude Opus 5.5 --- core/common/mixins.py | 9 +++++++-- core/integration_tests/tests_concepts.py | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index 927f3c160..e1ac3f19e 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -23,7 +23,8 @@ from core.common.constants import HEAD, ACCESS_TYPE_NONE, INCLUDE_FACETS, \ 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 + CHECKSUM_SMART_HEADER, SEARCH_LATEST_REPO_VERSION, SAME_STANDARD_CHECKSUM_ERROR, ACCESS_TYPE_VIEW, \ + INCLUDE_PARENT_CONCEPTS, INCLUDE_CHILD_CONCEPTS from core.common.permissions import HasPrivateAccess, HasOwnership, CanViewConceptDictionary, \ CanViewConceptDictionaryVersion, CanEditConceptDictionary from core.users.constants import LIST_UNPAGINATED_PERMISSION @@ -181,9 +182,13 @@ def __get_cached_data_if_any(self, request): return key_body, cache.get(key_body) or None, key_headers, cache.get(key_headers) or None def __can_cache(self): - # facets/search-stats are header driven and change the response shape, but are not part of the cache key + # 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 + 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 get(self, 'parent_resource.is_latest_version', False)) def list(self, request, *args, **kwargs): # pylint:disable=too-many-locals,too-many-branches,too-many-statements diff --git a/core/integration_tests/tests_concepts.py b/core/integration_tests/tests_concepts.py index 2ef60d9db..506433b29 100644 --- a/core/integration_tests/tests_concepts.py +++ b/core/integration_tests/tests_concepts.py @@ -3308,6 +3308,26 @@ def test_repo_version_concepts_are_served_from_cache(self): self.assertEqual(response.status_code, 200) self.assertEqual(response.data, [{'id': 'FromCache'}]) + 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']: + key = f'{body_key}?{param}=true' + url = f'/orgs/{self.org}/sources/MySource/v1/concepts/?{param}=true' + + response = self.client.get(url) + + self.assertEqual(response.status_code, 200) + 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 + cache.set(key, [{'id': 'FromCache'}]) + response = self.client.get(url) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data[0]['id'], 'MyConcept1') + def test_repo_version_concepts_facets_are_cached(self): body_key, _ = self.source_v1.get_concepts_cache_keys() facets_key = body_key + '?facetsOnly=true' From 349e19c4b217d08bc8fc861ecfdeef731a1e75f8 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:22:32 -0400 Subject: [PATCH 05/35] OpenConceptLab/ocl_online#224 | List only collection versions the requester can view A concept or mapping version's collection_versions (version detail, and versions lists with includeCollectionVersions) and the /collection-versions/ lists now show only public collection versions and those of the requester and their orgs (staff: all), using the collection version's own access. Co-Authored-By: Claude Opus 5.5 --- core/common/mixins.py | 20 ++++++++- core/concepts/serializers.py | 10 ++++- core/concepts/views.py | 5 ++- .../tests_public_read_access.py | 44 ++++++++++++++++++- core/mappings/serializers.py | 10 ++++- core/mappings/views.py | 5 ++- 6 files changed, 83 insertions(+), 11 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index e1ac3f19e..b8e684974 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -745,8 +745,24 @@ def source_versions(self): @property def collection_versions(self): - return set(self.expansion_set.exclude( - collection_version__version=HEAD).values_list('collection_version__uri', flat=True)) + return self.get_collection_versions() + + def get_collection_versions(self, user=None): + """URIs of the released collection versions holding this resource that the user can view.""" + expansions = self.filter_by_viewable_collection_version( + self.expansion_set.exclude(collection_version__version=HEAD), user) + return sorted(set(expansions.values_list('collection_version__uri', flat=True))) + + @staticmethod + def filter_by_viewable_collection_version(expansions, user): + """Expansions of collection versions the user can view: public ones, their own and their orgs' (staff: all).""" + if get(user, 'is_staff'): + return expansions + criteria = ~Q(collection_version__public_access=ACCESS_TYPE_NONE) + if get(user, 'is_authenticated'): + criteria |= Q(collection_version__user_id=user.id) | Q( + collection_version__organization__members__id=user.id) + return expansions.filter(criteria) @property def versions(self): diff --git a/core/concepts/serializers.py b/core/concepts/serializers.py index 5e8229b91..fb6fa2785 100644 --- a/core/concepts/serializers.py +++ b/core/concepts/serializers.py @@ -335,7 +335,7 @@ class Meta: class ConceptVersionListSerializer(ConceptListSerializer): previous_version_url = CharField(read_only=True, source='prev_version_uri') source_versions = ListField(read_only=True) - collection_versions = ListField(read_only=True) + collection_versions = SerializerMethodField() class Meta: model = Concept @@ -362,6 +362,9 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) + def get_collection_versions(self, obj): + return obj.get_collection_versions(get(self, 'context.request.user')) + class ConceptVersionCascadeSerializer(ConceptVersionListSerializer): class Meta: @@ -598,7 +601,7 @@ class ConceptVersionDetailSerializer(ModelSerializer): parent_concept_urls = ListField(read_only=True) child_concept_urls = ListField(read_only=True) source_versions = ListField(read_only=True) - collection_versions = ListField(read_only=True) + collection_versions = SerializerMethodField() references = SerializerMethodField() latest_source_version = CharField( source='latest_source_version.version', allow_null=True, allow_blank=True, read_only=True, required=False) @@ -689,6 +692,9 @@ def get_parent_concepts(self, obj): get_viewable_concepts(obj.parent_concepts.all(), get(self, 'context.request')), many=True).data return None + def get_collection_versions(self, obj): + return obj.get_collection_versions(get(self, 'context.request.user')) + class ConceptHierarchySerializer(ModelSerializer): uuid = CharField(source='id') diff --git a/core/concepts/views.py b/core/concepts/views.py index 5ef49744c..f2d2e207a 100644 --- a/core/concepts/views.py +++ b/core/concepts/views.py @@ -346,10 +346,11 @@ def get_queryset(self): instance = self.get_object() from core.collections.models import Collection - return Collection.objects.filter(id__in=instance.expansion_set.filter( + expansions = Concept.filter_by_viewable_collection_version(instance.expansion_set.filter( collection_version__organization_id=instance.parent.organization_id, collection_version__user_id=instance.parent.user_id - ).values_list('collection_version_id', flat=True)) + ), self.request.user) + return Collection.objects.filter(id__in=expansions.values_list('collection_version_id', flat=True)) def get(self, request, *args, **kwargs): return self.list(request, *args, **kwargs) diff --git a/core/integration_tests/tests_public_read_access.py b/core/integration_tests/tests_public_read_access.py index c0dfa8c61..60b54ab2c 100644 --- a/core/integration_tests/tests_public_read_access.py +++ b/core/integration_tests/tests_public_read_access.py @@ -1,14 +1,16 @@ from django.core.cache import cache from django.test import override_settings -from core.collections.tests.factories import OrganizationCollectionFactory +from core.collections.tests.factories import OrganizationCollectionFactory, ExpansionFactory from core.common.constants import ACCESS_TYPE_NONE from core.common.feeds import FeedFilterMixin, MAX_LIMIT, DEFAULT_LIMIT from core.common.tests import OCLAPITestCase from core.concepts.models import Concept from core.concepts.tests.factories import ConceptFactory +from core.mappings.tests.factories import MappingFactory from core.orgs.tests.factories import OrganizationFactory from core.sources.tests.factories import OrganizationSourceFactory +from core.users.models import UserProfile from core.users.tests.factories import UserProfileFactory @@ -57,6 +59,46 @@ def test_public_source_lookup_for_anyone(self): self.assertEqual([concept['id'] for concept in response.data], ['shown']) +class CollectionVersionsAccessTest(PrivateRepoReadAccessBaseTest): + def setUp(self): + super().setUp() + self.mapping = MappingFactory( + parent=self.public_source, from_concept=self.public_concept, to_concept=self.public_concept) + self.collection_urls = {} + for mnemonic, public_access in [('PublicColl', 'View'), ('PrivateColl', ACCESS_TYPE_NONE)]: + collection = OrganizationCollectionFactory( + organization=self.org, mnemonic=mnemonic, version='v1', public_access=public_access) + expansion = ExpansionFactory(collection_version=collection) + expansion.concepts.add(self.public_concept, self.public_concept.get_latest_version()) + expansion.mappings.add(self.mapping, self.mapping.get_latest_version()) + collection.refresh_from_db() + self.collection_urls[mnemonic] = collection.uri + + def get(self, url, user): + headers = {'HTTP_AUTHORIZATION': 'Token ' + user.get_token()} if user else {} + response = self.client.get(url, **headers) + self.assertEqual(response.status_code, 200, url) + return response.data + + def test_private_collection_versions_only_for_members_and_staff(self): + public_only = [self.collection_urls['PublicColl']] + both = sorted(self.collection_urls.values()) + admin = UserProfile.objects.get(username='ocladmin') + + for user, expected in [(None, public_only), (self.outsider, public_only), (self.member, both), (admin, both)]: + for resource in [self.public_concept, self.mapping]: + version = resource.get_latest_version() + + self.assertEqual(self.get(version.uri, user)['collection_versions'], expected, (user, resource)) + versions = self.get(resource.uri + 'versions/?includeCollectionVersions=true', user) + self.assertEqual( + [data['collection_versions'] for data in versions if data['uuid'] == str(version.id)], + [expected], (user, resource)) + self.assertEqual( + sorted(data['version_url'] for data in self.get(resource.uri + 'collection-versions/', user)), + expected, (user, resource)) + + class FeedAccessTest(PrivateRepoReadAccessBaseTest): def test_source_feed_only_for_public_sources(self): self.assertEqual(self.client.get(self.private_source.uri + 'atom/').status_code, 404) diff --git a/core/mappings/serializers.py b/core/mappings/serializers.py index 74a674056..8dde4783f 100644 --- a/core/mappings/serializers.py +++ b/core/mappings/serializers.py @@ -141,7 +141,7 @@ class Meta: class MappingVersionListSerializer(MappingListSerializer): previous_version_url = CharField(read_only=True, source='prev_version_uri') source_versions = ListField(read_only=True) - collection_versions = ListField(read_only=True) + collection_versions = SerializerMethodField() class Meta: model = Mapping @@ -162,6 +162,9 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) + def get_collection_versions(self, obj): + return obj.get_collection_versions(get(self, 'context.request.user')) + class MappingChecksumSerializer(AbstractResourceSerializer): id = CharField(source='mnemonic', read_only=True) @@ -318,10 +321,13 @@ def get_checksums(obj): class MappingVersionDetailSerializer(MappingDetailSerializer): previous_version_url = CharField(read_only=True, source='prev_version_uri') source_versions = ListField(read_only=True) - collection_versions = ListField(read_only=True) + collection_versions = SerializerMethodField() class Meta: model = Mapping fields = MappingDetailSerializer.Meta.fields + ( 'previous_version_url', 'source_versions', 'collection_versions', ) + + def get_collection_versions(self, obj): + return obj.get_collection_versions(get(self, 'context.request.user')) diff --git a/core/mappings/views.py b/core/mappings/views.py index e02c8aef3..4a058a975 100644 --- a/core/mappings/views.py +++ b/core/mappings/views.py @@ -278,10 +278,11 @@ def get_queryset(self): instance = self.get_object() from core.collections.models import Collection - return Collection.objects.filter(id__in=instance.expansion_set.filter( + expansions = Mapping.filter_by_viewable_collection_version(instance.expansion_set.filter( collection_version__organization_id=instance.parent.organization_id, collection_version__user_id=instance.parent.user_id - ).values_list('collection_version_id', flat=True)) + ), self.request.user) + return Collection.objects.filter(id__in=expansions.values_list('collection_version_id', flat=True)) def get(self, request, *args, **kwargs): return self.list(request, *args, **kwargs) From c76e54d032a323502d3909bfe51baaa235af77d8 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:24:50 -0400 Subject: [PATCH 06/35] OpenConceptLab/ocl_online#224 | Link a new concept or source only to mappings whose saver can view it Mappings to a concept or source that didn't exist yet keep the target as an external code and URL. When the concept or source is created, those mappings are linked only if the user who last saved them can view it, the same rule as when a mapping is saved. Also re-indexes the from-side mappings a new source links (the query filtered on to_source). Co-Authored-By: Claude Opus 5.5 --- core/common/models.py | 9 ++++ core/concepts/models.py | 6 ++- .../tests_cross_repo_access.py | 50 +++++++++++++++++++ core/sources/models.py | 8 +-- 4 files changed, 68 insertions(+), 5 deletions(-) diff --git a/core/common/models.py b/core/common/models.py index 4a54fbdec..58916e579 100644 --- a/core/common/models.py +++ b/core/common/models.py @@ -1033,6 +1033,15 @@ def has_view_access(self, user): return bool(user and user.is_authenticated and self.has_parent_edit_access(user)) + def get_view_access_criteria(self, user_field): + """has_view_access as a query on a UserProfile foreign key, e.g. the user who saved a row.""" + if self.public_can_view: + return Q() + criteria = Q(**{f'{user_field}__is_staff': True}) + if self.organization_id: + return criteria | Q(**{f'{user_field}__organizations__id': self.organization_id}) + return criteria | Q(**{f'{user_field}_id': self.user_id}) + @staticmethod def get_version_url_kwarg(): return 'version' diff --git a/core/concepts/models.py b/core/concepts/models.py index 5628da87e..436855271 100644 --- a/core/concepts/models.py +++ b/core/concepts/models.py @@ -1341,15 +1341,17 @@ def get_latest_versions_for_queryset(concepts_qs): 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 + can_view = self.parent.get_view_access_criteria('updated_by') Mapping.objects.filter( - to_concept_code=self.mnemonic, to_source_url__in=parent_uris, to_concept__isnull=True + can_view, to_concept_code=self.mnemonic, to_source_url__in=parent_uris, to_concept__isnull=True ).update(to_concept=self) Mapping.objects.filter( - from_concept_code=self.mnemonic, from_source_url__in=parent_uris, from_concept__isnull=True + can_view, from_concept_code=self.mnemonic, from_source_url__in=parent_uris, from_concept__isnull=True ).update(from_concept=self) @classmethod diff --git a/core/integration_tests/tests_cross_repo_access.py b/core/integration_tests/tests_cross_repo_access.py index 35e037aa2..c7a7b3e3a 100644 --- a/core/integration_tests/tests_cross_repo_access.py +++ b/core/integration_tests/tests_cross_repo_access.py @@ -8,6 +8,7 @@ from core.concepts.models import Concept from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory from core.orgs.tests.factories import OrganizationFactory +from core.sources.models import Source from core.sources.tests.factories import OrganizationSourceFactory, UserSourceFactory from core.users.models import UserProfile from core.users.tests.factories import UserProfileFactory @@ -253,6 +254,55 @@ def test_member_mapping_to_private_concept_is_linked(self): self.assertEqual(mapping.to_concept.versioned_object_id, self.private_concept.id) self.assertEqual(mapping.to_source_id, self.private_source.id) + def map_to(self, user, source, to_concept_url): + from_concept = ConceptFactory(parent=source) + response = self.client.post( + source.mappings_url, + {'map_type': 'SAME-AS', 'from_concept_url': from_concept.uri, 'to_concept_url': to_concept_url}, + HTTP_AUTHORIZATION='Token ' + user.get_token(), + format='json' + ) + self.assertEqual(response.status_code, 201, response.data) + return source.mappings.get(from_concept__versioned_object_id=from_concept.id, id=F('versioned_object_id')) + + def test_concept_created_later_is_linked_only_to_mappings_whose_saver_can_view_it(self): + member_source = OrganizationSourceFactory( + organization=self.private_org, mnemonic='MemberMapSource', public_access=ACCESS_TYPE_NONE) + outsider_mapping = self.map_to(self.outsider, self.outsider_source, self.private_source.uri + 'concepts/later/') + member_mapping = self.map_to(self.member, member_source, self.private_source.uri + 'concepts/later/') + public_mapping = self.map_to(self.outsider, self.outsider_source, self.public_source.uri + 'concepts/later/') + + for source in [self.private_source, self.public_source]: + response = self.client.post( + source.concepts_url, self.concept_payload('later'), + HTTP_AUTHORIZATION='Token ' + self.admin.get_token(), format='json') + self.assertEqual(response.status_code, 201, response.data) + + outsider_mapping.refresh_from_db() + self.assertIsNone(outsider_mapping.to_concept_id) + self.assertEqual(outsider_mapping.to_concept_code, 'later') + for mapping, source in [(member_mapping, self.private_source), (public_mapping, self.public_source)]: + mapping.refresh_from_db() + self.assertEqual(mapping.to_concept.parent_id, source.id) + + def test_source_created_later_is_linked_only_to_mappings_whose_saver_can_view_it(self): + member_source = OrganizationSourceFactory( + organization=self.private_org, mnemonic='MemberMapSource', public_access=ACCESS_TYPE_NONE) + outsider_mapping = self.map_to(self.outsider, self.outsider_source, '/orgs/PrivOrg/sources/NewPriv/concepts/x/') + member_mapping = self.map_to(self.member, member_source, '/orgs/PrivOrg/sources/NewPriv/concepts/x/') + + errors = Source.persist_new( + OrganizationSourceFactory.build( + organization=self.private_org, mnemonic='NewPriv', public_access=ACCESS_TYPE_NONE), + self.member, parent_resource=self.private_org) + self.assertEqual(errors, {}) + new_source = Source.objects.get(mnemonic='NewPriv', version='HEAD') + + outsider_mapping.refresh_from_db() + self.assertIsNone(outsider_mapping.to_source_id) + member_mapping.refresh_from_db() + self.assertEqual(member_mapping.to_source_id, new_source.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/sources/models.py b/core/sources/models.py index bf1b09b8d..079a473ba 100644 --- a/core/sources/models.py +++ b/core/sources/models.py @@ -245,16 +245,18 @@ def update_mappings(self): 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 + can_view = self.get_view_access_criteria('updated_by') to_queryset = Mapping.objects.filter(to_source_url__in=uris) from_queryset = Mapping.objects.filter(from_source_url__in=uris) - to_queryset.filter(to_source_id__isnull=True).update(to_source=self) - from_queryset.filter(from_source_id__isnull=True).update(from_source=self) + to_queryset.filter(can_view, to_source_id__isnull=True).update(to_source=self) + from_queryset.filter(can_view, from_source_id__isnull=True).update(from_source=self) Mapping.batch_index(to_queryset.filter(to_source=self), MappingDocument) - Mapping.batch_index(from_queryset.filter(to_source=self), MappingDocument) + Mapping.batch_index(from_queryset.filter(from_source=self), MappingDocument) def is_hierarchy_root_belonging_to_self(self): hierarchy_root = self.hierarchy_root From 16ed5e5359699bfb7f367a4af663d8107665f7b5 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:28:13 -0400 Subject: [PATCH 07/35] OpenConceptLab/ocl_online#224 | omitIfExistsIn ignores a repo the user can't view $cascade's omitIfExistsIn, and a collection reference's omit_if_exists_in, now apply only when the requester (for a reference, its creator) can view the repo; otherwise the expression is ignored, as if it didn't resolve. Co-Authored-By: Claude Opus 5.5 --- core/bundles/models.py | 16 +++++-- core/collections/models.py | 6 +++ core/concepts/views.py | 2 +- .../tests_cross_repo_access.py | 45 +++++++++++++++++++ 4 files changed, 65 insertions(+), 4 deletions(-) diff --git a/core/bundles/models.py b/core/bundles/models.py index fc8c90a61..f7dc8952d 100644 --- a/core/bundles/models.py +++ b/core/bundles/models.py @@ -11,8 +11,10 @@ class Bundle: - def __init__(self, root, repo_version, params=None, verbose=False, requested_url=None): # pylint: disable=too-many-arguments + def __init__( # pylint: disable=too-many-arguments + self, root, repo_version, params=None, verbose=False, requested_url=None, user=None): params = params or {} + self.user = user self.repo_version = repo_version self.params = params self.verbose = verbose @@ -96,7 +98,14 @@ def set_cascade_direction(self): self.reverse = self.params[CASCADE_DIRECTION_PARAM] in TRUTHY def set_omit_if_exists_in(self): - self.omit_if_exists_in = self.params.get(OMIT_IF_EXISTS_IN, None) or None + """A repo the user can't view is ignored, as if the expression didn't resolve.""" + omit_if_exists_in = self.params.get(OMIT_IF_EXISTS_IN, None) or None + if omit_if_exists_in: + from core.common.models import ConceptContainerModel + repo, _ = ConceptContainerModel.resolve_expression_to_version(omit_if_exists_in) + if get(repo, 'id') and not repo.has_view_access(self.user): + omit_if_exists_in = None + self.omit_if_exists_in = omit_if_exists_in def set_include_self(self): if INCLUDE_SELF in self.params: @@ -210,7 +219,8 @@ def clone( # pylint: disable=too-many-arguments params=parameters, verbose=is_verbose, repo_version=clone_from_source, - requested_url=requested_url + requested_url=requested_url, + user=user ) bundle.set_cascade_parameters() _parameters = {} diff --git a/core/collections/models.py b/core/collections/models.py index f403d2b8e..ffb013656 100644 --- a/core/collections/models.py +++ b/core/collections/models.py @@ -877,6 +877,12 @@ def get_concept_cascade_params(self, system_version=None): 'reverse', 'omit_if_exists_in']: if attr in self.cascade: cascade_params[attr] = get(self.cascade, attr) + omit_if_exists_in = cascade_params.get('omit_if_exists_in') + if omit_if_exists_in: + # a repo the reference's creator can't view is ignored, as if the expression didn't resolve + repo, _ = ConceptContainerModel.resolve_expression_to_version(omit_if_exists_in) + if get(repo, 'id') and not repo.has_view_access(self.created_by): + cascade_params.pop('omit_if_exists_in') map_types = get(self.cascade, 'map_types', None) exclude_map_types = get(self.cascade, 'exclude_map_types', None) return_map_types = get(self.cascade, 'return_map_types', None) diff --git a/core/concepts/views.py b/core/concepts/views.py index f2d2e207a..910a6725b 100644 --- a/core/concepts/views.py +++ b/core/concepts/views.py @@ -547,7 +547,7 @@ def get(self, request, **kwargs): # pylint: disable=unused-argument self.set_parent_resource(False) bundle = Bundle( root=instance, params=self.request.query_params, verbose=self.is_verbose(), - repo_version=self.parent_resource, requested_url=self.request.get_full_path() + repo_version=self.parent_resource, requested_url=self.request.get_full_path(), user=self.request.user ) bundle.cascade() return Response(BundleSerializer(bundle, context={'request': request}).data) diff --git a/core/integration_tests/tests_cross_repo_access.py b/core/integration_tests/tests_cross_repo_access.py index c7a7b3e3a..25ac96333 100644 --- a/core/integration_tests/tests_cross_repo_access.py +++ b/core/integration_tests/tests_cross_repo_access.py @@ -3,10 +3,13 @@ from mock import patch from core.bundles.models import Bundle +from core.collections.models import CollectionReference +from core.collections.tests.factories import OrganizationCollectionFactory, ExpansionFactory from core.common.constants import ACCESS_TYPE_NONE from core.common.tests import OCLAPITestCase from core.concepts.models import Concept 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 @@ -329,3 +332,45 @@ def test_edit_by_user_who_cannot_view_target_keeps_link(self): mapping.refresh_from_db() self.assertEqual(mapping.to_concept_id, linked_concept_id) self.assertEqual(mapping.to_source_id, linked_source_id) + + +class OmitIfExistsInAccessTest(CrossRepoAccessBaseTest): + def setUp(self): + super().setUp() + self.omitted = ConceptFactory(parent=self.public_source, mnemonic='omitted') + self.mapping = MappingFactory( + parent=self.public_source, from_concept=self.public_concept, to_concept=self.omitted) + self.private_collection = OrganizationCollectionFactory( + organization=self.private_org, mnemonic='PrivColl', public_access=ACCESS_TYPE_NONE) + expansion = ExpansionFactory(collection_version=self.private_collection) + self.private_collection.expansion_uri = expansion.uri + self.private_collection.save() + expansion.concepts.add(self.omitted.get_latest_version()) + + def cascade(self, user, omit_if_exists_in): + headers = {'HTTP_AUTHORIZATION': 'Token ' + user.get_token()} if user else {} + response = self.client.get( + self.public_concept.uri + '$cascade/?omitIfExistsIn=' + omit_if_exists_in, **headers) + self.assertEqual(response.status_code, 200) + return sorted(entry['url'] for entry in response.data['entry']) + + def test_cascade_ignores_omit_repo_the_requester_cannot_view(self): + everything = sorted([self.public_concept.uri, self.mapping.uri, self.omitted.uri]) + self.assertEqual(self.cascade(None, '/orgs/PrivOrg/collections/NoSuchColl/'), everything) + + for user in [None, self.outsider]: + self.assertEqual(self.cascade(user, self.private_collection.uri), everything, user) + + for user in [self.member, self.admin]: + self.assertEqual( + self.cascade(user, self.private_collection.uri), sorted([self.public_concept.uri, self.mapping.uri]), + user) + + def test_reference_ignores_omit_repo_its_creator_cannot_view(self): + for user, kept in [(self.outsider, False), (self.member, True), (self.admin, True), (None, False)]: + reference = CollectionReference( + expression=self.public_source.uri, + cascade={'method': 'sourcetoconcepts', 'omit_if_exists_in': self.private_collection.uri}, + created_by=user) + + self.assertEqual('omit_if_exists_in' in reference.get_concept_cascade_params(), kept, user) From d7b620182fa3dda1f8c8fd9322def780f84201ae Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:30:31 -0400 Subject: [PATCH 08/35] OpenConceptLab/ocl_online#224 | Read a concept's linked parents only when a new parent isn't viewable Updating a concept with parent_concept_urls read its previous version's parents (2 queries) on every update, even with no parents or with parents the user can view, which bulk imports pay per concept. They're now read only when some parent isn't viewable, to keep parents that are already linked. Co-Authored-By: Claude Opus 5.5 --- core/concepts/models.py | 18 +++++++++++------- .../tests_cross_repo_access.py | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/core/concepts/models.py b/core/concepts/models.py index 436855271..ec40e7dd2 100644 --- a/core/concepts/models.py +++ b/core/concepts/models.py @@ -651,7 +651,7 @@ def create_new_version_for( if has_parent_concept_uris_attr: parent_concept_uris = cls.get_viewable_parent_uris( - parent_concept_uris or [], user, prev_latest.parent_concept_urls if prev_latest else []) + parent_concept_uris or [], user, lambda: prev_latest.parent_concept_urls if prev_latest else []) else: parent_concept_uris = list(prev_latest.parent_concept_urls) if prev_latest else [] @@ -678,22 +678,23 @@ def create_new_version_for( @staticmethod def get_viewable_parent_uris(uris, user, linked_uris=None): """Only parents in repos the user can view can be linked. Parents already linked are kept, - and so are not-yet-existing ones pointing into a repo the user can view (linked later by the hierarchy task).""" + and so are not-yet-existing ones pointing into a repo the user can view (linked later by the hierarchy task). + `linked_uris` can be a callable, so it's only evaluated when some parent isn't viewable.""" from core.common.utils import to_parent_uri from core.sources.models import Source - linked_uris = set(linked_uris or []) - new_uris = [uri for uri in uris if uri not in linked_uris] + if not uris: + return [] viewable_uris = set() can_view_source = {} existing_uris = set() - for concept in Concept.objects.filter(uri__in=new_uris).select_related('parent') if new_uris else []: + for concept in Concept.objects.filter(uri__in=uris).select_related('parent'): existing_uris.add(concept.uri) if concept.parent_id not in can_view_source: can_view_source[concept.parent_id] = concept.parent.has_view_access(user) if can_view_source[concept.parent_id]: viewable_uris.add(concept.uri) repo_view_by_uri = {} - for uri in new_uris: + for uri in uris: if uri in existing_uris: continue repo_uri = to_parent_uri(uri) @@ -702,7 +703,10 @@ def get_viewable_parent_uris(uris, user, linked_uris=None): repo_view_by_uri[repo_uri] = bool(repo and repo.has_view_access(user)) if repo_view_by_uri[repo_uri]: viewable_uris.add(uri) - return [uri for uri in uris if uri in linked_uris or uri in viewable_uris] + if linked_uris and any(uri not in viewable_uris for uri in uris): + linked_uris = set(linked_uris() if callable(linked_uris) else linked_uris) + viewable_uris |= linked_uris + return [uri for uri in uris if uri in viewable_uris] def set_parent_concepts_from_uris(self, create_parent_version=True): parent_concepts = get(self, '_parent_concepts', []) diff --git a/core/integration_tests/tests_cross_repo_access.py b/core/integration_tests/tests_cross_repo_access.py index 25ac96333..d25fce697 100644 --- a/core/integration_tests/tests_cross_repo_access.py +++ b/core/integration_tests/tests_cross_repo_access.py @@ -173,6 +173,23 @@ def test_include_parent_concepts_hides_private_parents(self): self.assertEqual( sorted(concept['id'] for concept in response.data['parent_concepts']), ['public', 'secret']) + def test_viewable_parent_uris_reads_linked_parents_only_when_a_parent_is_not_viewable(self): + def not_needed(): + raise AssertionError('linked parents were read') + + with self.assertNumQueries(0): + self.assertEqual(Concept.get_viewable_parent_uris([], self.outsider, not_needed), []) + self.assertEqual( + Concept.get_viewable_parent_uris([self.public_concept.uri], self.outsider, not_needed), + [self.public_concept.uri]) + for linked, expected in [ + ([self.private_concept.uri], [self.private_concept.uri, self.public_concept.uri]), + ([], [self.public_concept.uri])]: + self.assertEqual( + Concept.get_viewable_parent_uris( + [self.private_concept.uri, self.public_concept.uri], self.outsider, lambda linked=linked: linked), + expected) + def test_parents_and_children_lists_hide_private_concepts(self): child = self.persist_child('listedchild', [self.private_concept.uri, self.public_concept.uri]) private_child = Concept.persist_new({ From cdc989d5d9c4069e76e5ffda8050f5e032cb2341 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:28:13 -0400 Subject: [PATCH 09/35] OpenConceptLab/ocl_online#224 | GraphQL concepts query returns only concepts the requester can view A source-scoped query checks view access on the source version, and one the requester cannot view gets the same error as a source that does not exist. A global query (conceptIds or text) and the mappings listed for each concept use the same rule as the REST concept and mapping lists (apply_user_criteria; staff see everything), and the Elasticsearch search behind `query` applies get_visible_repo_criteria, so totalCount counts only viewable hits. Co-Authored-By: Claude Opus 5.5 --- core/graphql/queries.py | 47 +++++--- .../tests/test_concepts_from_source.py | 113 +++++++++++++++++- core/graphql/tests/test_query_helpers.py | 24 ++-- 3 files changed, 153 insertions(+), 31 deletions(-) diff --git a/core/graphql/queries.py b/core/graphql/queries.py index e96b6ac7e..14a3ae035 100644 --- a/core/graphql/queries.py +++ b/core/graphql/queries.py @@ -14,6 +14,7 @@ from strawberry.exceptions import GraphQLError from core.common.constants import HEAD +from core.common.search import get_visible_repo_criteria from core.concepts.documents import ConceptDocument from core.concepts.models import Concept from core.mappings.models import Mapping @@ -67,7 +68,8 @@ class ConceptSearchResult: ) -async def resolve_source_version(org: str, source: str, version: Optional[str]) -> Source: +async def resolve_source_version(org: str, source: str, version: Optional[str], user) -> Source: + """The org's source version; one the user cannot view is reported as not found.""" filters = {'organization__mnemonic': org} target_version = version or HEAD instance = await sync_to_async(Source.get_version)(source, target_version, filters) @@ -75,7 +77,7 @@ async def resolve_source_version(org: str, source: str, version: Optional[str]) if not instance and version is None: instance = await sync_to_async(Source.find_latest_released_version_by)({**filters, 'mnemonic': source}) - if not instance: + if not instance or not await sync_to_async(instance.has_view_access)(user): raise GraphQLError( f"Source '{source}' with version '{version or 'HEAD'}' was not found for org '{org}'." ) @@ -103,13 +105,23 @@ def build_mapping_prefetch(source_version: Source) -> Prefetch: return Prefetch('mappings_from', queryset=mapping_qs, to_attr='graphql_mappings') -def build_global_mapping_prefetch() -> Prefetch: +def build_global_base_queryset(user): + """Active concepts across all repositories the user can view.""" + queryset = Concept.objects.filter(is_active=True, retired=False) + return queryset if user.is_staff else Concept.apply_user_criteria(queryset, user) + + +def build_global_mapping_prefetch(user) -> Prefetch: + """Mappings from each concept, in repositories the user can view.""" + mapping_qs = Mapping.objects.filter( + from_concept_id__isnull=False, + is_active=True, + retired=False, + ) + if not user.is_staff: + mapping_qs = Mapping.apply_user_criteria(mapping_qs, user) mapping_qs = ( - Mapping.objects.filter( - from_concept_id__isnull=False, - is_active=True, - retired=False, - ) + mapping_qs .select_related('to_source', 'to_concept', 'to_concept__parent') .order_by('map_type', 'to_concept_code', 'to_concept__mnemonic') .distinct() @@ -354,6 +366,7 @@ def concept_ids_from_es( query: str, source_version: Optional[Source], pagination: Optional[dict], + user, ) -> Optional[tuple[list[int], int]]: trimmed = query.strip() if not trimmed: @@ -368,6 +381,9 @@ def concept_ids_from_es( else: search = search.filter('term', source_version=source_version.version) search = search.filter('term', retired=False) + visible_repo_criteria = get_visible_repo_criteria(user) + if visible_repo_criteria: + search = search.filter(visible_repo_criteria) should_queries = [ ES_Q('match', id={'query': trimmed, 'boost': 6, 'operator': 'AND'}), @@ -425,14 +441,15 @@ async def concepts_for_ids( return await sync_to_async(list)(qs), total -async def concepts_for_query( +async def concepts_for_query( # pylint: disable=too-many-arguments base_qs, query: str, source_version: Source, pagination: Optional[dict], mapping_prefetch: Prefetch, + user, ) -> tuple[List[Concept], int]: - es_result = await sync_to_async(concept_ids_from_es)(query, source_version, pagination) + es_result = await sync_to_async(concept_ids_from_es)(query, source_version, pagination, user) if es_result is not None: concept_ids, total = es_result if not concept_ids: @@ -488,16 +505,17 @@ async def concepts( # pylint: disable=too-many-arguments,too-many-locals raise GraphQLError('Either conceptIds or query must be provided.') pagination = normalize_pagination(page, limit) + user = info.context.user if org and source: - source_version = await resolve_source_version(org, source, version) + source_version = await resolve_source_version(org, source, version, user) base_qs = build_base_queryset(source_version) mapping_prefetch = build_mapping_prefetch(source_version) else: - # Global search across all repositories + # Global search across all repositories the user can view source_version = None - base_qs = Concept.objects.filter(is_active=True, retired=False) - mapping_prefetch = build_global_mapping_prefetch() + base_qs = build_global_base_queryset(user) + mapping_prefetch = build_global_mapping_prefetch(user) if concept_ids_param: concepts, total = await concepts_for_ids(base_qs, concept_ids_param, pagination, mapping_prefetch) @@ -508,6 +526,7 @@ async def concepts( # pylint: disable=too-many-arguments,too-many-locals source_version, pagination, mapping_prefetch, + user, ) serialized = await sync_to_async(serialize_concepts)(concepts) diff --git a/core/graphql/tests/test_concepts_from_source.py b/core/graphql/tests/test_concepts_from_source.py index c4caf4013..83d30765e 100644 --- a/core/graphql/tests/test_concepts_from_source.py +++ b/core/graphql/tests/test_concepts_from_source.py @@ -1,11 +1,15 @@ import json import os +import uuid from unittest import mock from django.test import override_settings +from rest_framework.authtoken.models import Token -from core.common.constants import HEAD +from core.common.constants import ACCESS_TYPE_NONE, HEAD from core.common.tests import OCLTestCase +from core.concepts.documents import ConceptDocument +from core.concepts.models import Concept from core.concepts.tests.factories import ( ConceptDescriptionFactory, ConceptFactory, @@ -103,17 +107,36 @@ def setUp(self): self.concept1.extras = {**(self.concept1.extras or {}), 'is_set': False} self.concept1.save(update_fields=['extras']) - def _execute(self, query: str, variables: dict): + def _post(self, query: str, variables: dict, user=None): + auth_header = auth_header_for_token(Token.objects.get_or_create(user=user)[0]) if user else self.auth_header response = self.client.post( '/graphql/', data=json.dumps({'query': query, 'variables': variables}), content_type='application/json', - HTTP_AUTHORIZATION=self.auth_header, + HTTP_AUTHORIZATION=auth_header, ) - payload = response.json() + return response.status_code, response.json() + + def _execute(self, query: str, variables: dict, user=None): + status, payload = self._post(query, variables, user) if 'errors' in payload: self.fail(payload['errors']) - return response.status_code, payload['data'] + return status, payload['data'] + + def _create_private_source(self, source_mnemonic='GQLPRIVATE', concept_name='Private concept'): + """A private org source with one concept and a mapping from the public concept1, and an org member.""" + private_org = OrganizationFactory(mnemonic='GQLPRIVORG', public_access=ACCESS_TYPE_NONE) + private_source = OrganizationSourceFactory( + organization=private_org, mnemonic=source_mnemonic, version=HEAD, public_access=ACCESS_TYPE_NONE) + private_concept = ConceptFactory( + parent=private_source, mnemonic='gql-private-1', public_access=ACCESS_TYPE_NONE, + names=[ConceptNameFactory.build(name=concept_name, locale='en', locale_preferred=True)]) + MappingFactory( + parent=private_source, from_concept=self.concept1, to_concept=private_concept, map_type='Narrower Than', + public_access=ACCESS_TYPE_NONE) + member, _ = create_user_with_token(username='graphql-private-member', super_user=self.super_user) + private_org.members.add(member) + return private_source, private_concept, member def tearDown(self): if self._old_async_flag is None: @@ -557,3 +580,83 @@ def test_serialize_concepts_includes_metadata_for_retired_concepts(self): self.assertEqual(serialized.metadata.updated_by, retired_user.username) self.assertEqual(serialized.metadata.created_at, format_datetime_for_api(retired.created_at)) self.assertEqual(serialized.metadata.updated_at, format_datetime_for_api(retired.updated_at)) + + def test_private_source_is_reported_as_not_found(self): + private_source, private_concept, member = self._create_private_source() + query = """ + query ($org: String, $source: String, $conceptIds: [String!]) { + concepts(org: $org, source: $source, conceptIds: $conceptIds) { + totalCount + results { conceptId } + } + } + """ + variables = {'org': 'GQLPRIVORG', 'source': private_source.mnemonic, 'conceptIds': [private_concept.mnemonic]} + + _, missing = self._post(query, {**variables, 'source': 'NOSUCHSOURCE'}) + _, private = self._post(query, variables) + + self.assertIsNone(private['data']) + self.assertEqual( + [error['message'] for error in private['errors']], + [error['message'].replace('NOSUCHSOURCE', private_source.mnemonic) for error in missing['errors']]) + for user in [member, self.super_user]: + _, data = self._execute(query, variables, user) + self.assertEqual(data['concepts']['totalCount'], 1) + self.assertEqual(data['concepts']['results'][0]['conceptId'], private_concept.mnemonic) + + def test_global_concept_ids_lookup_lists_only_viewable_concepts_and_mappings(self): + _, private_concept, member = self._create_private_source() + query = """ + query ($conceptIds: [String!]) { + concepts(conceptIds: $conceptIds) { + totalCount + results { conceptId mappings { mapType } } + } + } + """ + variables = {'conceptIds': [self.concept1.mnemonic, private_concept.mnemonic]} + + for user, concept_ids, map_types in [ + (None, {self.concept1.mnemonic}, {'Same As'}), + (member, {self.concept1.mnemonic, private_concept.mnemonic}, {'Same As', 'Narrower Than'}), + (self.super_user, {self.concept1.mnemonic, private_concept.mnemonic}, {'Same As', 'Narrower Than'})]: + _, data = self._execute(query, variables, user) + results = data['concepts']['results'] + self.assertEqual({result['conceptId'] for result in results}, concept_ids, user) + self.assertEqual(data['concepts']['totalCount'], len(results), user) + self.assertEqual( + {mapping['mapType'] for result in results for mapping in result['mappings']}, map_types, user) + + @mock.patch('core.graphql.queries.fallback_db_search', side_effect=AssertionError('expected an ES search')) + def test_query_search_lists_and_counts_only_viewable_concepts(self, _fallback_mock): + self.patch_concept_es_mapping_for_ci() + token = f'zyq{uuid.uuid4().hex[:10]}' + public_concept = ConceptFactory( + parent=self.source, mnemonic='gql-public-1', + names=[ConceptNameFactory.build(name=f'{token} public', locale='en', locale_preferred=True)]) + # a private source with the same name as the public one, in another org + _, private_concept, member = self._create_private_source(self.source.mnemonic, f'{token} private') + ConceptDocument().update( + Concept.objects.filter(mnemonic__in=[public_concept.mnemonic, private_concept.mnemonic]), refresh=True) + query = """ + query ($org: String, $source: String, $text: String!) { + concepts(org: $org, source: $source, query: $text) { + totalCount + results { conceptId } + } + } + """ + + for user, concept_ids in [ + (None, {public_concept.mnemonic}), + (member, {public_concept.mnemonic, private_concept.mnemonic}), + (self.super_user, {public_concept.mnemonic, private_concept.mnemonic})]: + _, data = self._execute(query, {'text': token}, user) + results = data['concepts']['results'] + self.assertEqual({result['conceptId'] for result in results}, concept_ids, user) + self.assertEqual(data['concepts']['totalCount'], len(results), user) + + _, data = self._execute( + query, {'org': self.organization.mnemonic, 'source': self.source.mnemonic, 'text': token}) + self.assertEqual(data['concepts']['totalCount'], 1) diff --git a/core/graphql/tests/test_query_helpers.py b/core/graphql/tests/test_query_helpers.py index e0148904b..3d3d99c9b 100644 --- a/core/graphql/tests/test_query_helpers.py +++ b/core/graphql/tests/test_query_helpers.py @@ -250,7 +250,7 @@ def test_resolve_source_version_and_base_queries(self): ) with patch('core.graphql.queries.Source.get_version', return_value=self.source): success = async_to_sync(resolve_source_version)( - self.organization.mnemonic, self.source.mnemonic, None + self.organization.mnemonic, self.source.mnemonic, None, self.audit_user ) self.assertEqual(success, self.source) @@ -258,17 +258,17 @@ def test_resolve_source_version_and_base_queries(self): 'core.graphql.queries.Source.find_latest_released_version_by', return_value=fallback_only ): resolved = async_to_sync(resolve_source_version)( - self.organization.mnemonic, fallback_only.mnemonic, None + self.organization.mnemonic, fallback_only.mnemonic, None, self.audit_user ) self.assertEqual(resolved, fallback_only) with self.assertRaises(GraphQLError): async_to_sync(resolve_source_version)( - self.organization.mnemonic, 'missing-source', 'v-does-not-exist' + self.organization.mnemonic, 'missing-source', 'v-does-not-exist', self.audit_user ) base_qs = build_base_queryset(self.source) mapping_prefetch = build_mapping_prefetch(self.source) - global_prefetch = build_global_mapping_prefetch() + global_prefetch = build_global_mapping_prefetch(self.audit_user) self.assertIsNotNone(mapping_prefetch) self.assertIsNotNone(global_prefetch) @@ -287,7 +287,7 @@ def test_resolve_source_version_error_path_and_pagination_defaults(self): 'core.graphql.queries.Source.find_latest_released_version_by', return_value=None ): with self.assertRaises(GraphQLError): - async_to_sync(resolve_source_version)('ORG', 'SRC', None) + async_to_sync(resolve_source_version)('ORG', 'SRC', None, self.audit_user) self.assertIsNone(normalize_pagination(None, None)) self.assertFalse(has_next(10, None)) @@ -387,7 +387,7 @@ def test_datatype_helpers(self): self.assertIsNone(format_datetime_for_api(None)) def test_concept_ids_from_es_paths(self): - ids, total = concept_ids_from_es(' ', self.source, None) + ids, total = concept_ids_from_es(' ', self.source, None, self.audit_user) self.assertEqual(ids, []) self.assertEqual(total, 0) @@ -426,12 +426,12 @@ def execute(self): 'core.graphql.queries.ConceptDocument.search', return_value=FakeSearch([self.concept1.id, self.concept2.id]), ): - ids, total = concept_ids_from_es('search text', self.source, {'start': 0, 'end': 1}) + ids, total = concept_ids_from_es('search text', self.source, {'start': 0, 'end': 1}, self.audit_user) self.assertEqual(ids, [self.concept1.id]) self.assertEqual(total, 2) with patch('core.graphql.queries.ConceptDocument.search', side_effect=Exception('boom')): - self.assertIsNone(concept_ids_from_es('text', self.source, None)) + self.assertIsNone(concept_ids_from_es('text', self.source, None, self.audit_user)) def test_fallback_and_concepts_queries(self): base_qs = build_base_queryset(self.source) @@ -453,20 +453,20 @@ def test_fallback_and_concepts_queries(self): with patch('core.graphql.queries.concept_ids_from_es', return_value=([self.concept2.id], 1)): concepts, total = async_to_sync(concepts_for_query)( - base_qs, 'anything', self.source, None, mapping_prefetch + base_qs, 'anything', self.source, None, mapping_prefetch, self.audit_user ) self.assertEqual(total, 1) self.assertEqual(concepts[0].id, self.concept2.id) with patch('core.graphql.queries.concept_ids_from_es', return_value=None): concepts, total = async_to_sync(concepts_for_query)( - base_qs, 'UTIL', self.source, normalize_pagination(1, 1), mapping_prefetch + base_qs, 'UTIL', self.source, normalize_pagination(1, 1), mapping_prefetch, self.audit_user ) self.assertGreaterEqual(total, 1) with patch('core.graphql.queries.concept_ids_from_es', return_value=([], 2)): concepts, total = async_to_sync(concepts_for_query)( - base_qs, 'UTIL', self.source, None, mapping_prefetch + base_qs, 'UTIL', self.source, None, mapping_prefetch, self.audit_user ) self.assertEqual(total, 2) self.assertEqual(concepts, []) @@ -480,7 +480,7 @@ def test_query_concepts_auth_and_results(self): with self.assertRaises(GraphQLError): async_to_sync(Query().concepts)(info_invalid, query='test') - info_valid = SimpleNamespace(context=SimpleNamespace(auth_status='valid')) + info_valid = SimpleNamespace(context=SimpleNamespace(auth_status='valid', user=self.audit_user)) with self.assertRaises(GraphQLError): async_to_sync(Query().concepts)(info_valid) From 04f17e6905b79a5de2a924a65af01cd100e067f0 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:29:58 -0400 Subject: [PATCH 10/35] OpenConceptLab/ocl_online#224 | CodeSystem $lookup and $validate-code treat code systems the requester can't view as missing $lookup and $validate-code resolve `system`/`url` to the first code system version the requester can view (with its HEAD, which governs its concepts), skipping the rest as if they didn't exist. $lookup gives the same not-found response for a code in a code system the requester can't view as for a code system that doesn't exist, and $validate-code returns result false for signed-in requesters too, not only anonymous ones. Co-Authored-By: Claude Opus 5.5 --- core/code_systems/tests.py | 55 ++++++++++++++++++++++++++++++++++++++ core/code_systems/views.py | 27 ++++++++++++++++--- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/core/code_systems/tests.py b/core/code_systems/tests.py index c1bf49b1b..a2d3e52c3 100644 --- a/core/code_systems/tests.py +++ b/core/code_systems/tests.py @@ -7,6 +7,7 @@ from core.code_systems.converter import CodeSystemConverter from core.code_systems.serializers import CodeSystemDetailSerializer from core.code_systems.views import CodeSystemLookupNotFoundError +from core.common.constants import ACCESS_TYPE_NONE from core.common.tests import OCLTestCase from core.concepts.models import Concept from core.concepts.tests.factories import ConceptFactory @@ -259,6 +260,60 @@ def test_lookup_for_code_system_with_new_canonical(self): {'name': 'version', 'valueString': self.org_source_v2.version}, {'name': 'display', 'valueString': self.concept_1.display_name}]})) + @patch('core.sources.models.index_source_concepts', Mock(__name__='index_source_concepts')) + @patch('core.sources.models.index_source_mappings', Mock(__name__='index_source_mappings')) + def test_private_code_system_answers_as_missing_for_non_viewers(self): + private_org = OrganizationFactory() + private_source = OrganizationSourceFactory( + organization=private_org, canonical_url='/private/url', public_access=ACCESS_TYPE_NONE) + concept = ConceptFactory(parent=private_source, names=1, names__name='private_concept_name') + Source.persist_new_version(OrganizationSourceFactory.build( + version='v1', mnemonic=private_source.mnemonic, organization=private_org, + public_access=ACCESS_TYPE_NONE), private_source.created_by) + member = UserProfileFactory() + private_org.members.add(member) + outsider = {'HTTP_AUTHORIZATION': 'Token ' + self.user_token} + validate_url = f'/fhir/CodeSystem/$validate-code/?url=/private/url&code={concept.mnemonic}' + lookup_url = '/fhir/CodeSystem/$lookup/?system={}&code={}' + not_found = self.client.get(lookup_url.format('/unknown/url', concept.mnemonic)) + self.assertEqual(not_found.status_code, 400) + + for headers in [{}, outsider]: + for url in [validate_url, f'{validate_url}&display={concept.display_name}']: + response = self.client.get(url, **headers) + self.assertEqual(response.status_code, 200, (url, headers)) + self.assertFalse(response.data['parameter'][0]['valueBoolean'], (url, headers)) + for code in [concept.mnemonic, 'missing']: + response = self.client.get(lookup_url.format('/private/url', code), **headers) + self.assertEqual(response.status_code, 400, (code, headers)) + self.assertEqual(response.content, not_found.content, (code, headers)) + + member_headers = {'HTTP_AUTHORIZATION': 'Token ' + member.get_token()} + response = self.client.get(validate_url, **member_headers) + self.assertTrue(response.data['parameter'][0]['valueBoolean']) + response = self.client.get(lookup_url.format('/private/url', concept.mnemonic), **member_headers) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data['parameter'][2]['valueString'], concept.display_name) + + @patch('core.sources.models.index_source_concepts', Mock(__name__='index_source_concepts')) + @patch('core.sources.models.index_source_mappings', Mock(__name__='index_source_mappings')) + def test_private_code_system_does_not_hide_public_one_with_same_url(self): + private_source = UserSourceFactory(user=self.user, public_access=ACCESS_TYPE_NONE, canonical_url='/shared/url') + Source.persist_new_version(UserSourceFactory.build( + version='v1', mnemonic=private_source.mnemonic, user=self.user), private_source.created_by) + public_source = OrganizationSourceFactory(canonical_url='/shared/url') + concept = ConceptFactory(parent=public_source) + Source.persist_new_version(OrganizationSourceFactory.build( + version='v1', mnemonic=public_source.mnemonic, organization=public_source.organization), + public_source.created_by) + + response = self.client.get(f'/fhir/CodeSystem/$lookup/?system=/shared/url&code={concept.mnemonic}') + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data['parameter'][0]['valueString'], public_source.mnemonic) + + response = self.client.get(f'/fhir/CodeSystem/$validate-code/?url=/shared/url&code={concept.mnemonic}') + self.assertTrue(response.data['parameter'][0]['valueBoolean']) + @patch('core.sources.models.index_source_concepts', Mock(__name__='index_source_concepts')) @patch('core.sources.models.index_source_mappings', Mock(__name__='index_source_mappings')) def test_post_code_system_without_concepts(self): diff --git a/core/code_systems/views.py b/core/code_systems/views.py index a03bd3ab7..53ebd3187 100644 --- a/core/code_systems/views.py +++ b/core/code_systems/views.py @@ -1,7 +1,7 @@ import logging from django.db.models import F -from rest_framework.exceptions import ValidationError, NotAuthenticated +from rest_framework.exceptions import ValidationError, NotAuthenticated, PermissionDenied from core.bundles.serializers import FHIRBundleSerializer from core.code_systems.serializers import CodeSystemDetailSerializer, \ @@ -18,6 +18,16 @@ logger = logging.getLogger('oclapi') +def get_viewable_repo(repos, user): + """The first of these repo versions the user can view, skipping the rest as if they didn't exist. Its HEAD, + which governs access to its concepts, must be viewable too.""" + for repo in repos.order_by('id'): + head = repo.head + if head and repo.has_view_access(user) and head.has_view_access(user): + return repo + return None + + class CodeSystemListView(SourceListView): serializer_class = CodeSystemDetailSerializer @@ -96,14 +106,22 @@ def get_queryset(self): else: source = source.filter(is_latest_version=True).exclude(version=HEAD) + source = get_viewable_repo(source, self.request.user) if source: - queryset = queryset.filter(sources=source.first(), mnemonic=code) + queryset = queryset.filter(sources=source, mnemonic=code) if not queryset.exists(): raise CodeSystemLookupNotFoundError(code) return queryset raise CodeSystemLookupNotFoundError() + def get_object(self, queryset=None): + """A code the requester can't view gives the same answer as a code system that doesn't exist.""" + try: + return super().get_object(queryset) + except (NotAuthenticated, PermissionDenied) as ex: + raise CodeSystemLookupNotFoundError() from ex + def get_serializer(self, instance=None): # pylint: disable=arguments-differ if instance: return ParametersSerializer.from_concept(instance) @@ -154,12 +172,13 @@ def get_queryset(self): source = source.filter(version=version) else: source = source.filter(is_latest_version=True).exclude(version=HEAD) + source = get_viewable_repo(source, self.request.user) if code: if not source: return queryset.none() - queryset = queryset.filter(sources=source.first(), mnemonic=code) + queryset = queryset.filter(sources=source, mnemonic=code) if display and queryset: instance = queryset.first() @@ -187,7 +206,7 @@ def get_object(self, queryset=None): if instance: try: self.check_object_permissions(self.request, instance) - except NotAuthenticated: + except (NotAuthenticated, PermissionDenied): return { 'parameter': [ {'name': 'result', 'valueBoolean': False} From 9c37bb54ac548d1cec441d44768eb91c51e4dc71 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:34:10 -0400 Subject: [PATCH 11/35] OpenConceptLab/ocl_online#224 | ValueSet $validate-code reads only value sets the requester can view $validate-code reads only the value sets the request names: the `url` value set and the route's collection. Query params no longer pick another owner or collection; a `version` param only narrows the route's collection. Each value set, its HEAD and every version read must be viewable by the requester; one that doesn't resolve or can't be viewed gives result false, the same as a code that isn't in it. The owner-less route reads the collections with that id the requester can view. Without `url`, the global route still checks the code system alone. A `display` with a code that isn't in the value set now gives result false instead of a server error. Co-Authored-By: Claude Opus 5.5 --- core/value_sets/tests.py | 68 +++++++++++++++++++++++++++++++- core/value_sets/views.py | 83 ++++++++++++++++++++++------------------ 2 files changed, 113 insertions(+), 38 deletions(-) diff --git a/core/value_sets/tests.py b/core/value_sets/tests.py index 7315b51b1..f68e35f21 100644 --- a/core/value_sets/tests.py +++ b/core/value_sets/tests.py @@ -1,7 +1,7 @@ from mock.mock import patch, Mock from core.collections.models import CollectionReference, Collection -from core.collections.tests.factories import OrganizationCollectionFactory, ExpansionFactory +from core.collections.tests.factories import OrganizationCollectionFactory, ExpansionFactory, UserCollectionFactory from core.common.constants import ACCESS_TYPE_NONE from core.common.tests import OCLAPITestCase from core.concepts.documents import ConceptDocument @@ -581,6 +581,72 @@ def test_validate_code_private_value_set_only_for_members_and_staff(self): self.assertEqual(response.status_code, 200, url) self.assertEqual(response.data['parameter'][0]['valueBoolean'], expected, (url, headers)) + def add_reference(self, collection, concept): + if not collection.expansion_uri: + collection.expansion_uri = ExpansionFactory(collection_version=collection).uri + collection.save() + collection.add_references([ + CollectionReference( + expression=concept.uri, collection=collection, code=concept.mnemonic, + system=concept.parent.uri, version='v2' + ), + ]) + + def test_validate_code_reads_only_value_sets_the_requester_can_view(self): + self.add_reference(self.collection, self.concept_1) + private_org = OrganizationFactory() + member = UserProfileFactory() + private_org.members.add(member) + private_collection = OrganizationCollectionFactory( + organization=private_org, mnemonic='priv', public_access=ACCESS_TYPE_NONE) + self.add_reference(private_collection, self.concept_2) + self.add_reference(OrganizationCollectionFactory(organization=private_org, mnemonic='pub'), self.concept_1) + owner = UserProfileFactory() + self.add_reference( + UserCollectionFactory(user=owner, mnemonic='upriv', public_access=ACCESS_TYPE_NONE), self.concept_2) + query = f'system=http://some/url&systemVersion={self.org_source_v2.version}&code=' + private_org_params = f'org={private_org.mnemonic}&collection=priv' + + for headers in [{}, {'HTTP_AUTHORIZATION': 'Token ' + self.user_token}]: + for url in [ + f'/orgs/{self.org.mnemonic}/ValueSet/c1/$validate-code/?{private_org_params}&{query}', + f'/orgs/{private_org.mnemonic}/ValueSet/pub/$validate-code/?collection=priv&{query}', + f'/fhir/ValueSet/upriv/$validate-code/?{query}', + f'/fhir/ValueSet/upriv/$validate-code/?user={owner.username}&{query}', + f'/fhir/ValueSet/c1/$validate-code/?user={owner.username}&collection=upriv&{query}', + ]: + response = self.client.get(url + self.concept_2.mnemonic, **headers) + self.assertEqual(response.status_code, 200, (url, headers)) + self.assertFalse(response.data['parameter'][0]['valueBoolean'], (url, headers)) + + # Without `url`, the global route checks the code system alone; other params don't pick a value set + for concept in [self.concept_1, self.concept_2]: + response = self.client.get( + f'/fhir/ValueSet/$validate-code/?{private_org_params}&{query}{concept.mnemonic}', **headers) + self.assertTrue(response.data['parameter'][0]['valueBoolean'], (concept.mnemonic, headers)) + + for url, headers in [ + (f'/orgs/{private_org.mnemonic}/ValueSet/priv/$validate-code/?{query}', member), + (f'/fhir/ValueSet/priv/$validate-code/?{query}', member), + (f'/fhir/ValueSet/upriv/$validate-code/?{query}', owner), + ]: + response = self.client.get( + url + self.concept_2.mnemonic, HTTP_AUTHORIZATION='Token ' + headers.get_token()) + self.assertTrue(response.data['parameter'][0]['valueBoolean'], url) + response = self.client.get(f'/fhir/ValueSet/c1/$validate-code/?{query}{self.concept_1.mnemonic}') + self.assertTrue(response.data['parameter'][0]['valueBoolean']) + + def test_validate_code_with_display_for_code_not_in_value_set(self): + self.add_reference(self.collection, self.concept_1) + + response = self.client.get( + f'/orgs/{self.org.mnemonic}/ValueSet/c1/$validate-code/' + f'?system=http://some/url&systemVersion={self.org_source_v2.version}&code=missing&display=x' + ) + + self.assertEqual(response.status_code, 200) + self.assertFalse(response.data['parameter'][0]['valueBoolean']) + def test_expand(self): self.client.post( f'/users/{self.user.mnemonic}/ValueSet/', diff --git a/core/value_sets/views.py b/core/value_sets/views.py index c9432c3be..b18bab53b 100644 --- a/core/value_sets/views.py +++ b/core/value_sets/views.py @@ -2,17 +2,16 @@ from django.core.exceptions import ValidationError from rest_framework import status -from rest_framework.exceptions import PermissionDenied from rest_framework.response import Response from core.bundles.serializers import FHIRBundleSerializer -from core.code_systems.views import CodeSystemValidateCodeView +from core.code_systems.views import CodeSystemValidateCodeView, get_viewable_repo from core.collections.models import Collection from core.collections.views import CollectionListView, CollectionRetrieveUpdateDestroyView, \ CollectionVersionExpansionsView -from core.common.constants import HEAD +from core.common.constants import HEAD, LATEST from core.common.fhir_helpers import translate_fhir_query -from core.concepts.views import ConceptRetrieveUpdateDestroyView +from core.concepts.models import Concept from core.sources.models import Source from core.value_sets.serializers import ValueSetDetailSerializer, \ ValueSetExpansionParametersSerializer, ValueSetExpansionSerializer @@ -52,32 +51,37 @@ def get_detail_serializer(self, obj): class ValueSetValidateCodeView(CodeSystemValidateCodeView): - INVALID_RESULT = {'parameter': [{'name': 'result', 'valueBoolean': False}]} - - def get_value_sets(self): - value_sets = [] - url = self.get_parameters().get('url') - if url: - value_sets.append(Collection.objects.filter( - canonical_url=url, is_latest_version=True).exclude(version=HEAD).first()) - collection = self.kwargs.get('collection') - if collection: - owner_filters = {'organization__mnemonic': self.kwargs['org']} if 'org' in self.kwargs else { - 'user__username': self.kwargs.get('user')} - value_sets.append(Collection.get_version(collection, self.kwargs.get('version', HEAD), owner_filters)) - return [value_set for value_set in value_sets if value_set] - - def get_object(self, queryset=None): - """A value set the requester can't view gives the same answer as a code that isn't in it.""" - if any(not value_set.has_view_access(self.request.user) for value_set in self.get_value_sets()): - return self.INVALID_RESULT - try: - return super().get_object(queryset) - except PermissionDenied: - return self.INVALID_RESULT + CONTAINER_PARAMS = ('user', 'org', 'source', 'collection', 'version') + + def get_route_value_set_versions(self): + """Ids of the route's collection versions the requester can view, narrowed by a `version` query param. The + owner-less route reads every collection with that id the requester can view.""" + user = self.request.user + heads = Collection.objects.filter( + mnemonic=self.kwargs['collection'], version=HEAD).select_related('organization') + if 'org' in self.kwargs: + heads = heads.filter(organization__mnemonic=self.kwargs['org']) + elif 'user' in self.kwargs: + heads = heads.filter(user__username=self.kwargs['user']) + version = self.request.query_params.get('version') + version_ids = [] + for head in heads: + has_private_access = bool(user.is_authenticated and head.has_parent_edit_access(user)) + if not has_private_access and not head.public_can_view: + continue + versions = head.versions.only('id', 'public_access') + if version == LATEST: + versions = versions.filter(released=True)[:1] + elif version: + versions = versions.filter(version=version) + version_ids += [repo.id for repo in versions if has_private_access or repo.public_can_view] + return version_ids def get_queryset(self): - queryset = super(ConceptRetrieveUpdateDestroyView, self).get_queryset() + """Reads only the value sets the request names (`url` and the route's collection), never ones other query + params pick; a value set that doesn't resolve or the requester can't view reads nothing.""" + params = {key: value for key, value in self.params.items() if key not in self.CONTAINER_PARAMS} + queryset = Concept.get_base_queryset(params) parameters = self.get_parameters() @@ -88,28 +92,33 @@ def get_queryset(self): system_version = parameters.get('systemVersion') if url: - collection = Collection.objects.filter(canonical_url=url).exclude(version=HEAD)\ - .filter(is_latest_version=True) + collection = get_viewable_repo( + Collection.objects.filter(canonical_url=url, is_latest_version=True).exclude(version=HEAD), + self.request.user) if not collection: return queryset.none() - queryset = queryset.filter(references__collection=collection.first()) + queryset = queryset.filter(references__collection=collection) + + if 'collection' in self.kwargs: + version_ids = self.get_route_value_set_versions() + if not version_ids: + return queryset.none() + queryset = queryset.filter(expansion_set__collection_version__in=version_ids) if code and system: concept_source = Source.objects.filter(canonical_url=system) - if not concept_source: - return queryset.none() if system_version: concept_source = concept_source.filter(version=system_version) else: concept_source = concept_source.filter(is_latest_version=True).exclude(version=HEAD) - if concept_source: - queryset = queryset.filter(sources=concept_source.first(), mnemonic=code) - else: + concept_source = get_viewable_repo(concept_source, self.request.user) + if not concept_source: return queryset.none() + queryset = queryset.filter(sources=concept_source, mnemonic=code) if display: instance = queryset.first() - if display not in (instance.name, instance.display_name): + if not instance or display not in (instance.name, instance.display_name): return queryset.none() return queryset From 83af49b2b3420b64d65d9b97804e0f7bcbc7b82b Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:23:16 -0400 Subject: [PATCH 12/35] OpenConceptLab/ocl_online#224 | Following feed: a followed user's private events go to staff followers only A user's private events are about orgs and repos the follower may not be able to view, so the following scope now shows a followed user's public events only, unless the follower is staff. Private events of a followed org or repo still go to followers who can view it, and org members still get their orgs' private events in the orgs scope. Co-Authored-By: Claude Opus 5.5 --- core/events/models.py | 11 +++++++++-- core/integration_tests/tests_events.py | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/core/events/models.py b/core/events/models.py index 6074afe20..b7ad856e3 100644 --- a/core/events/models.py +++ b/core/events/models.py @@ -40,12 +40,19 @@ def get_events_for_following(cls, following_queryset, private=False, **event_kwa queryset = cls.objects.none() for following in following_queryset: events = following.following.events.filter(**event_kwargs) - # private events only for objects the follower can still view - if not private or not following.follower.can_view(following.following): + if not private or not cls.can_follower_view_private_events(following.follower, following.following): events = events.filter(public=True) queryset = queryset.union(events) return queryset + @staticmethod + def can_follower_view_private_events(follower, followed): + """Private events of an org or repo go to followers who can view it. A user's private events are about + orgs and repos the follower may not view, so only staff get them (org members get them in the orgs scope).""" + if followed.__class__.__name__ == 'UserProfile': + return follower.is_staff + return follower.can_view(followed) + @classmethod def get_user_organization_events(cls, user, private=False): org_uris = list(user.organizations.values_list('uri', flat=True)) diff --git a/core/integration_tests/tests_events.py b/core/integration_tests/tests_events.py index 2b9b4fc86..9fe02d2b1 100644 --- a/core/integration_tests/tests_events.py +++ b/core/integration_tests/tests_events.py @@ -235,6 +235,31 @@ def test_get_following_events(self): # pylint:disable=too-many-statements,too-m ] self.assertIn(gotham_gazette_crime_repo.url, created_urls) + def test_get_following_events_without_followed_users_private_events(self): + followed = UserProfileFactory(username='followed') + follower = UserProfileFactory(username='follower') + member = UserProfileFactory(username='member') + staff = UserProfileFactory(username='staff', is_staff=True) + private_org = OrganizationFactory(mnemonic='private-org', public_access='None') + private_org.members.add(followed, member) + joined_private_org_event = EventFactory( + object_url=followed.url, referenced_object_url=private_org.url, actor=followed, event_type='Joined', + public=False) + for user in [follower, member, staff]: + user.follow(followed) + + def get_event_ids(user, scopes): + response = self.client.get( + f'/users/{user.username}/events/?scopes={scopes}', HTTP_AUTHORIZATION=f'Token {user.get_token()}') + self.assertEqual(response.status_code, 200) + return [event['id'] for event in response.data] + + self.assertNotIn(joined_private_org_event.id, get_event_ids(follower, 'following')) + self.assertNotIn(joined_private_org_event.id, get_event_ids(follower, 'all')) + self.assertNotIn(joined_private_org_event.id, get_event_ids(member, 'following')) + self.assertIn(joined_private_org_event.id, get_event_ids(member, 'orgs')) + self.assertIn(joined_private_org_event.id, get_event_ids(staff, 'following')) + 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') From 873fe731def1fd055ccb3e8ec49d97cadea1f128 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:24:52 -0400 Subject: [PATCH 13/35] OpenConceptLab/ocl_online#224 | A user's following list shows only follows of objects the requester can view /users//following/ and the includeFollowing field of user details and verbose user lists now leave out follows of orgs, repos and repo content the requester can't view. The user themselves and staff still get every follow. UserProfile.can_user_view(user, obj) answers can_view for any requester, anonymous included, and can_view delegates to it. Co-Authored-By: Claude Opus 5.5 --- core/integration_tests/tests_users.py | 33 +++++++++++++++++++++++++++ core/users/models.py | 19 ++++++++++++--- core/users/serializers.py | 6 ++++- core/users/tests/tests.py | 22 ++++++++++++++++++ core/users/views.py | 2 +- 5 files changed, 77 insertions(+), 5 deletions(-) diff --git a/core/integration_tests/tests_users.py b/core/integration_tests/tests_users.py index b5406a63f..0e1b69d28 100644 --- a/core/integration_tests/tests_users.py +++ b/core/integration_tests/tests_users.py @@ -1270,6 +1270,39 @@ 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): + 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') + member = UserProfileFactory(username='private-member') + private_org.members.add(member) + outsider = UserProfileFactory(username='private-outsider') + staff = UserProfileFactory(username='private-staff', is_staff=True) + for followed in [private_org, private_source, public_org]: + member.follow(followed) + all_urls = sorted([private_org.url, private_source.url, public_org.url]) + + def get_followed_urls(url, user=None): + headers = {'HTTP_AUTHORIZATION': 'Token ' + user.get_token()} if user else {} + response = self.client.get(url, **headers) + self.assertEqual(response.status_code, 200) + follows = response.data + if url.startswith('/users/?'): + 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) + + following_url = f'/users/{member.username}/following/' + detail_url = f'/users/{member.username}/?includeFollowing=true' + list_url = '/users/?verbose=true&includeFollowing=true' + for url in [following_url, list_url]: + self.assertEqual(get_followed_urls(url), [public_org.url]) + for url in [following_url, detail_url, list_url]: + self.assertEqual(get_followed_urls(url, outsider), [public_org.url]) + self.assertEqual(get_followed_urls(url, member), all_urls) + self.assertEqual(get_followed_urls(url, staff), all_urls) + def test_post(self): follower = UserProfileFactory(username='follower') followed = UserProfileFactory(username='followed') diff --git a/core/users/models.py b/core/users/models.py index f09337594..1d6cb3328 100644 --- a/core/users/models.py +++ b/core/users/models.py @@ -423,15 +423,28 @@ def is_member_of_org(self, org_mnemonic): def can_view(self, obj): """Whether this user can view a user, an org, a repo or content inside a repo.""" + return self.can_user_view(self, obj) + + @staticmethod + def can_user_view(user, obj): + """Whether `user` (anonymous or None for the public) can view a user, an org, a repo or content in a repo.""" from core.orgs.models import Organization - if self.is_staff or isinstance(obj, UserProfile): + if getattr(user, 'is_staff', False) or isinstance(obj, UserProfile): return True if isinstance(obj, Organization): - return obj.public_can_view or obj.is_member(self) + return obj.public_can_view or bool(user and user.is_authenticated and obj.is_member(user)) repo = obj if hasattr(obj, 'has_view_access') else ( getattr(obj, 'collection_version', None) or getattr(obj, 'collection', None) or getattr(obj, 'parent', None)) - return bool(repo and hasattr(repo, 'has_view_access') and repo.has_view_access(self)) + 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 and staff get them all.""" + follows = self.following.prefetch_related('following') + 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 def follow(self, following): self.following.create(following_id=following.id, following_type=ContentType.objects.get_for_model(following)) diff --git a/core/users/serializers.py b/core/users/serializers.py index 2c9f5b9f4..83581e93d 100644 --- a/core/users/serializers.py +++ b/core/users/serializers.py @@ -204,7 +204,7 @@ class UserDetailSerializer(PrivateUserFieldsMixin, AbstractResourceSerializer): deactivated_at = serializers.DateTimeField(read_only=True) pins = serializers.SerializerMethodField() followers = FollowerSerializer(many=True, read_only=True) - following = FollowingSerializer(many=True, read_only=True) + following = serializers.SerializerMethodField() class Meta: model = UserProfile @@ -273,6 +273,10 @@ 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 + def validate_username(self, value): from core.services.auth.core import AuthService # Under SSO, Keycloak owns usernames and OIDC sign-in matches accounts on them, so no caller can change one. diff --git a/core/users/tests/tests.py b/core/users/tests/tests.py index b0e51976e..d3ffa6ae8 100644 --- a/core/users/tests/tests.py +++ b/core/users/tests/tests.py @@ -12,6 +12,7 @@ from core.common.tasks import send_user_verification_email, send_user_reset_password_email from core.common.tests import OCLTestCase, OCLAPITestCase, PREVIEW_GROUP_NAME from core.orgs.models import Organization +from core.orgs.tests.factories import OrganizationFactory from core.sources.tests.factories import OrganizationSourceFactory from core.users.constants import USER_OBJECT_TYPE, OCL_SERVERS_GROUP, MAPPER_USE_PERMISSION, \ MAPPER_AI_ASSISTANT_PERMISSION, MAPPER_SCISPACY_PERMISSION, STAFF_GROUP, SUPERADMIN_GROUP @@ -304,6 +305,27 @@ def test_user_without_preview_group_has_no_mapper_permission(self): self.assertFalse(user.has_perm(MAPPER_USE_PERMISSION)) + def test_can_user_view(self): + from django.contrib.auth.models import AnonymousUser + from core.concepts.tests.factories import ConceptFactory + member, outsider, staff = UserProfileFactory(), UserProfileFactory(), UserProfileFactory(is_staff=True) + private_org = OrganizationFactory(public_access=ACCESS_TYPE_NONE) + private_org.members.add(member) + private_source = OrganizationSourceFactory(organization=private_org, public_access=ACCESS_TYPE_NONE) + private_concept = ConceptFactory(parent=private_source) + public_source = OrganizationSourceFactory(organization=private_org) + + for obj in [private_org, private_source, private_concept]: + for user in [None, AnonymousUser(), outsider]: + self.assertFalse(UserProfile.can_user_view(user, obj)) + self.assertFalse(outsider.can_view(obj)) + for user in [member, staff]: + self.assertTrue(UserProfile.can_user_view(user, obj)) + self.assertTrue(user.can_view(obj)) + for obj in [public_source, member]: + for user in [None, AnonymousUser(), outsider]: + self.assertTrue(UserProfile.can_user_view(user, obj)) + class TokenAuthenticationViewTest(OCLAPITestCase): def test_post(self): diff --git a/core/users/views.py b/core/users/views.py index d5945f142..a95eab19b 100644 --- a/core/users/views.py +++ b/core/users/views.py @@ -620,7 +620,7 @@ class UserFollowingListView(AbstractFollowerFollowedView, ListAPIView): serializer_class = FollowingSerializer def get_queryset(self): - return self.get_object().following.filter() + return self.get_object().get_following_visible_to(self.request.user) def post(self, request, *args, **kwargs): # pylint: disable=unused-argument follower = self.get_object() From db55cb1feaa0b960539de25881c741abf0d7cda0 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:29:02 -0400 Subject: [PATCH 14/35] OpenConceptLab/ocl_online#224 | Follow events of objects the public can't view are private Followed and Unfollowed events now get public=False when the followed org, repo or repo content isn't publicly viewable, like Created and Joined events already do. The user themselves, staff, and followers who can view the object still see them. Migration events/0011 makes existing Followed/Unfollowed events private when they reference a non-public org, or a non-public HEAD source or collection (or content under one). Reverse is a no-op. Co-Authored-By: Claude Opus 5.5 --- .../migrations/0011_private_follow_events.py | 43 +++++++ core/events/tests/tests.py | 31 +++++ core/integration_tests/tests_events.py | 110 ++++++++++-------- core/users/models.py | 2 + core/users/tests/tests.py | 19 +++ 5 files changed, 158 insertions(+), 47 deletions(-) create mode 100644 core/events/migrations/0011_private_follow_events.py diff --git a/core/events/migrations/0011_private_follow_events.py b/core/events/migrations/0011_private_follow_events.py new file mode 100644 index 000000000..c5866d485 --- /dev/null +++ b/core/events/migrations/0011_private_follow_events.py @@ -0,0 +1,43 @@ +import re + +from django.db import migrations + +REPO_URI = re.compile(r'^/(?:orgs|users)/[^/]+/(?:sources|collections)/[^/]+/') + + +def get_private_uris(model, uris, **kwargs): + # 'Edit' is retired and reads as 'View' + return { + uri for uri, public_access in model.objects.filter(uri__in=uris, **kwargs).values_list('uri', 'public_access') + if (public_access or '').lower() not in ('view', 'edit') + } + + +# Followed/Unfollowed events of an org or repo the public can't view (or of content in such a repo) are private. +def make_follow_events_of_private_objects_private(apps, schema_editor): # pylint: disable=unused-argument + events = apps.get_model('events', 'Event').objects.filter(event_type__in=['Followed', 'Unfollowed'], public=True) + urls = set(filter(None, events.values_list('referenced_object_url', flat=True))) + repo_uris = {url: match.group(0) for url in urls if (match := REPO_URI.match(url))} + + private_uris = get_private_uris(apps.get_model('orgs', 'Organization'), urls) + for app_label, model_name in [('sources', 'Source'), ('collections', 'Collection')]: + private_uris |= get_private_uris( + apps.get_model(app_label, model_name), set(repo_uris.values()), version='HEAD') + + private_urls = [url for url in urls if url in private_uris or repo_uris.get(url) in private_uris] + if private_urls: + events.filter(referenced_object_url__in=private_urls).update(public=False) + + +class Migration(migrations.Migration): + + dependencies = [ + ('events', '0010_auto_20250910_0142'), + ('orgs', '0022_retire_public_edit_access'), + ('sources', '0046_retire_public_edit_access'), + ('collections', '0071_retire_public_edit_access'), + ] + + operations = [ + migrations.RunPython(make_follow_events_of_private_objects_private, migrations.RunPython.noop), + ] diff --git a/core/events/tests/tests.py b/core/events/tests/tests.py index f9c54153e..f182b1bab 100644 --- a/core/events/tests/tests.py +++ b/core/events/tests/tests.py @@ -1,3 +1,6 @@ +import importlib + +from django.apps import apps from django.utils import timezone from core.common.tests import OCLTestCase @@ -109,3 +112,31 @@ def test_clean_fields(self): self.user_event.clean_fields() self.assertEqual(self.user_event.public, False) + + +class PrivateFollowEventsMigrationTest(OCLTestCase): + def test_migration_makes_follow_events_of_private_objects_private(self): + migration = importlib.import_module('core.events.migrations.0011_private_follow_events') + follower = UserProfileFactory() + private_org = OrganizationFactory(public_access='None') + public_org = OrganizationFactory() + private_source = OrganizationSourceFactory(organization=public_org, public_access='None') + public_source = OrganizationSourceFactory(organization=private_org) + private_concept = ConceptFactory(parent=private_source) + public_concept = ConceptFactory(parent=public_source) + urls = [ + follower.url, private_org.url, public_org.url, private_source.url, public_source.url, + private_concept.url, public_concept.url, '/orgs/deleted-org/' + ] + events = { + (event_type, url): Event.objects.create( + object_url=follower.url, referenced_object_url=url, event_type=event_type, actor=follower) + for url in urls for event_type in ['Followed', 'Unfollowed', 'Created'] + } + + migration.make_follow_events_of_private_objects_private(apps, None) + + private_urls = [private_org.url, private_source.url, private_concept.url] + for (event_type, url), event in events.items(): + event.refresh_from_db() + self.assertEqual(event.public, event_type == 'Created' or url not in private_urls, (event_type, url)) diff --git a/core/integration_tests/tests_events.py b/core/integration_tests/tests_events.py index 9fe02d2b1..7a5d03d41 100644 --- a/core/integration_tests/tests_events.py +++ b/core/integration_tests/tests_events.py @@ -172,11 +172,11 @@ def test_get_following_events(self): # pylint:disable=too-many-statements,too-m # 1. Mario's event of joining Gotham Gazette # 2. Penguin's event of joining Gotham # 3. Joker's event of joining Gotham - # 4. Bruce's follow of Gotham Gazette Crime Deptt - # 5. Bruce's follow of Gotham Gazette - # 6. Joker's event of following Bruce - # 7. OCL's event of creation of Gotham Gazette - self.assertEqual(len(response.data), 7) + # 4. Bruce's follow of Gotham Gazette + # 5. Joker's event of following Bruce + # 6. OCL's event of creation of Gotham Gazette + # Not Bruce's follow of the private Gotham Gazette Crime Deptt: that event is private + self.assertEqual(len(response.data), 6) self.assertEqual(response.data[0]['object']['url'], mario.url) self.assertEqual(response.data[0]['referenced_object']['url'], gotham_gazette.url) self.assertEqual(response.data[1]['object']['url'], penguin.url) @@ -184,13 +184,11 @@ def test_get_following_events(self): # pylint:disable=too-many-statements,too-m self.assertEqual(response.data[2]['object']['url'], joker.url) self.assertEqual(response.data[2]['referenced_object']['url'], gotham.url) self.assertEqual(response.data[3]['object']['url'], bruce.url) - self.assertEqual(response.data[3]['referenced_object']['url'], gotham_gazette_crime_repo.url) - self.assertEqual(response.data[4]['object']['url'], bruce.url) - self.assertEqual(response.data[4]['referenced_object']['url'], gotham_gazette.url) - self.assertEqual(response.data[5]['object']['url'], joker.url) - self.assertEqual(response.data[5]['referenced_object']['url'], bruce.url) - self.assertEqual(response.data[6]['object']['url'], '/users/ocladmin/') - self.assertEqual(response.data[6]['referenced_object']['url'], gotham_gazette.url) + self.assertEqual(response.data[3]['referenced_object']['url'], gotham_gazette.url) + self.assertEqual(response.data[4]['object']['url'], joker.url) + self.assertEqual(response.data[4]['referenced_object']['url'], bruce.url) + self.assertEqual(response.data[5]['object']['url'], '/users/ocladmin/') + self.assertEqual(response.data[5]['referenced_object']['url'], gotham_gazette.url) response = self.client.get( '/users/bruce/events/?scopes=following', @@ -201,12 +199,12 @@ def test_get_following_events(self): # pylint:disable=too-many-statements,too-m # 1. Mario's event of joining Gotham Gazette # 2. Penguin's event of joining Gotham # 3. Joker's event of joining Gotham - # 4. Bruce's follow of Gotham Gazette Crime Deptt - # 5. Bruce's follow of Gotham Gazette - # 6. Joker's event of following Bruce - # 7. OCL's event of creation of Gotham Gazette - # Not the private creation of Gotham Gazette Crime Deptt: Bruce can't view that repo - self.assertEqual(len(response.data), 7) + # 4. Bruce's follow of Gotham Gazette + # 5. Joker's event of following Bruce + # 6. OCL's event of creation of Gotham Gazette + # Not the private creation of Gotham Gazette Crime Deptt, nor Bruce's (private) follow of it: + # Bruce can't view that repo + self.assertEqual(len(response.data), 6) self.assertEqual(response.data[0]['object']['url'], mario.url) self.assertEqual(response.data[0]['referenced_object']['url'], gotham_gazette.url) self.assertEqual(response.data[1]['object']['url'], penguin.url) @@ -214,13 +212,11 @@ def test_get_following_events(self): # pylint:disable=too-many-statements,too-m self.assertEqual(response.data[2]['object']['url'], joker.url) self.assertEqual(response.data[2]['referenced_object']['url'], gotham.url) self.assertEqual(response.data[3]['object']['url'], bruce.url) - self.assertEqual(response.data[3]['referenced_object']['url'], gotham_gazette_crime_repo.url) - self.assertEqual(response.data[4]['object']['url'], bruce.url) - self.assertEqual(response.data[4]['referenced_object']['url'], gotham_gazette.url) - self.assertEqual(response.data[5]['object']['url'], joker.url) - self.assertEqual(response.data[5]['referenced_object']['url'], bruce.url) - self.assertEqual(response.data[6]['object']['url'], '/users/ocladmin/') - self.assertEqual(response.data[6]['referenced_object']['url'], gotham_gazette.url) + self.assertEqual(response.data[3]['referenced_object']['url'], gotham_gazette.url) + self.assertEqual(response.data[4]['object']['url'], joker.url) + self.assertEqual(response.data[4]['referenced_object']['url'], bruce.url) + self.assertEqual(response.data[5]['object']['url'], '/users/ocladmin/') + self.assertEqual(response.data[5]['referenced_object']['url'], gotham_gazette.url) mario.follow(gotham_gazette_crime_repo) response = self.client.get( @@ -260,6 +256,28 @@ def get_event_ids(user, scopes): self.assertIn(joined_private_org_event.id, get_event_ids(member, 'orgs')) self.assertIn(joined_private_org_event.id, get_event_ids(staff, 'following')) + def test_get_follow_events_of_private_objects(self): + private_org = OrganizationFactory(mnemonic='private-org', public_access='None') + private_source = OrganizationSourceFactory(organization=private_org, public_access='None') + public_org = OrganizationFactory(mnemonic='public-org') + member = UserProfileFactory(username='member') + private_org.members.add(member) + outsider = UserProfileFactory(username='outsider') + staff = UserProfileFactory(username='staff', is_staff=True) + for followed in [private_org, private_source, public_org]: + member.follow(followed) + + def get_followed_urls(user=None): + headers = {'HTTP_AUTHORIZATION': f'Token {user.get_token()}'} if user else {} + response = self.client.get('/users/member/events/', **headers) + self.assertEqual(response.status_code, 200) + return sorted(event['referenced_object']['url'] for event in response.data) + + self.assertEqual(get_followed_urls(), [public_org.url]) + self.assertEqual(get_followed_urls(outsider), [public_org.url]) + for user in [member, staff]: + self.assertEqual(get_followed_urls(user), sorted([private_org.url, private_source.url, public_org.url])) + 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') @@ -432,7 +450,8 @@ def test_get_all_events(self): # pylint:disable=too-many-statements,too-many-lo '/users/bruce/events/?scopes=all' ) self.assertEqual(response.status_code, 200) - self.assertEqual(len(response.data), 12) + # not Bruce's follow of the private Gotham Gazette Crime Unit: that event is private + self.assertEqual(len(response.data), 11) self.assertEqual(response.data[0]['object']['url'], mario.url) self.assertEqual(response.data[0]['referenced_object']['url'], gotham_gazette.url) self.assertEqual(response.data[1]['object']['url'], penguin.url) @@ -446,17 +465,15 @@ def test_get_all_events(self): # pylint:disable=too-many-statements,too-many-lo self.assertEqual(response.data[5]['object']['url'], bruce.url) self.assertEqual(response.data[5]['referenced_object']['url'], wayne_corp.url) self.assertEqual(response.data[6]['object']['url'], bruce.url) - self.assertEqual(response.data[6]['referenced_object']['url'], gotham_gazette_crime_repo.url) - self.assertEqual(response.data[7]['object']['url'], bruce.url) - self.assertEqual(response.data[7]['referenced_object']['url'], gotham_gazette.url) - self.assertEqual(response.data[8]['object']['url'], joker.url) - self.assertEqual(response.data[8]['referenced_object']['url'], bruce.url) + self.assertEqual(response.data[6]['referenced_object']['url'], gotham_gazette.url) + self.assertEqual(response.data[7]['object']['url'], joker.url) + self.assertEqual(response.data[7]['referenced_object']['url'], bruce.url) + self.assertEqual(response.data[8]['object']['url'], '/users/ocladmin/') + self.assertEqual(response.data[8]['referenced_object']['url'], gotham_gazette.url) self.assertEqual(response.data[9]['object']['url'], '/users/ocladmin/') - self.assertEqual(response.data[9]['referenced_object']['url'], gotham_gazette.url) + self.assertEqual(response.data[9]['referenced_object']['url'], gotham.url) self.assertEqual(response.data[10]['object']['url'], '/users/ocladmin/') - self.assertEqual(response.data[10]['referenced_object']['url'], gotham.url) - self.assertEqual(response.data[11]['object']['url'], '/users/ocladmin/') - self.assertEqual(response.data[11]['referenced_object']['url'], wayne_corp.url) + self.assertEqual(response.data[10]['referenced_object']['url'], wayne_corp.url) response = self.client.get( '/users/bruce/events/?scopes=all', # all = orgs + following @@ -464,8 +481,9 @@ def test_get_all_events(self): # pylint:disable=too-many-statements,too-many-lo ) self.assertEqual(response.status_code, 200) bruce_all_events = response.data - # not the private creation of Gotham Gazette Crime Unit: Bruce follows it but can't view it - self.assertEqual(len(bruce_all_events), 15) + # not the private creation of Gotham Gazette Crime Unit, nor Bruce's (private) follow of it: + # Bruce follows it but can't view it + self.assertEqual(len(bruce_all_events), 14) self.assertEqual(response.data[0]['object']['url'], mario.url) self.assertEqual(response.data[0]['referenced_object']['url'], gotham_gazette.url) self.assertEqual(response.data[1]['object']['url'], penguin.url) @@ -483,19 +501,17 @@ def test_get_all_events(self): # pylint:disable=too-many-statements,too-many-lo self.assertEqual(response.data[7]['object']['url'], bruce.url) self.assertEqual(response.data[7]['referenced_object']['url'], bat_cave.url) self.assertEqual(response.data[8]['object']['url'], bruce.url) - self.assertEqual(response.data[8]['referenced_object']['url'], gotham_gazette_crime_repo.url) - self.assertEqual(response.data[9]['object']['url'], bruce.url) - self.assertEqual(response.data[9]['referenced_object']['url'], gotham_gazette.url) - self.assertEqual(response.data[10]['object']['url'], joker.url) - self.assertEqual(response.data[10]['referenced_object']['url'], bruce.url) + self.assertEqual(response.data[8]['referenced_object']['url'], gotham_gazette.url) + self.assertEqual(response.data[9]['object']['url'], joker.url) + self.assertEqual(response.data[9]['referenced_object']['url'], bruce.url) + self.assertEqual(response.data[10]['object']['url'], '/users/ocladmin/') + self.assertEqual(response.data[10]['referenced_object']['url'], gotham_gazette.url) self.assertEqual(response.data[11]['object']['url'], '/users/ocladmin/') - self.assertEqual(response.data[11]['referenced_object']['url'], gotham_gazette.url) + self.assertEqual(response.data[11]['referenced_object']['url'], gotham.url) self.assertEqual(response.data[12]['object']['url'], '/users/ocladmin/') - self.assertEqual(response.data[12]['referenced_object']['url'], gotham.url) + self.assertEqual(response.data[12]['referenced_object']['url'], bat_cave.url) self.assertEqual(response.data[13]['object']['url'], '/users/ocladmin/') - self.assertEqual(response.data[13]['referenced_object']['url'], bat_cave.url) - self.assertEqual(response.data[14]['object']['url'], '/users/ocladmin/') - self.assertEqual(response.data[14]['referenced_object']['url'], wayne_corp.url) + self.assertEqual(response.data[13]['referenced_object']['url'], wayne_corp.url) response = self.client.get( '/users/bruce/events/?scopes=orgs,following', diff --git a/core/users/models.py b/core/users/models.py index 1d6cb3328..94b592443 100644 --- a/core/users/models.py +++ b/core/users/models.py @@ -455,6 +455,7 @@ def follow(self, following): event_type=Event.FOLLOWED, object_url=self.url, referenced_object_url=following.url, + public=self.can_user_view(None, following), # private for objects the public can't view ) def unfollow(self, following): @@ -467,6 +468,7 @@ def unfollow(self, following): event_type=Event.UNFOLLOWED, object_url=self.url, referenced_object_url=following.url, + public=self.can_user_view(None, following), ) def set_groups(self, groups, verify=True, _save=True): diff --git a/core/users/tests/tests.py b/core/users/tests/tests.py index d3ffa6ae8..4ef2fad5d 100644 --- a/core/users/tests/tests.py +++ b/core/users/tests/tests.py @@ -326,6 +326,25 @@ def test_can_user_view(self): for user in [None, AnonymousUser(), outsider]: self.assertTrue(UserProfile.can_user_view(user, obj)) + def test_follow_events_of_private_objects_are_private(self): + follower = UserProfileFactory() + followed_user = UserProfileFactory() + private_source = OrganizationSourceFactory(public_access=ACCESS_TYPE_NONE) + public_source = OrganizationSourceFactory() + + for followed in [followed_user, private_source, public_source]: + follower.follow(followed) + follower.unfollow(followed) + + self.assertEqual( + sorted(follower.events.values_list('event_type', 'referenced_object_url', 'public')), + sorted([ + ('Followed', followed_user.url, True), ('Unfollowed', followed_user.url, True), + ('Followed', private_source.url, False), ('Unfollowed', private_source.url, False), + ('Followed', public_source.url, True), ('Unfollowed', public_source.url, True), + ]) + ) + class TokenAuthenticationViewTest(OCLAPITestCase): def test_post(self): From 80efbcef74c852d1a97d4ffd7a253b1de3570661 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:19:56 -0400 Subject: [PATCH 15/35] OpenConceptLab/ocl_online#224 | User list: lastLoginSince/lastLoginBefore filters apply only for staff Last login is staff-only account data, so the two filters are now ignored for non-staff and anonymous requests, the same way the staff-only isSuperuser/isStaff/isAdmin filters already are. Co-Authored-By: Claude Opus 5.5 --- core/common/swagger_parameters.py | 6 ++++-- core/integration_tests/tests_users.py | 23 +++++++++++++++++++++++ core/users/views.py | 6 ++++-- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/core/common/swagger_parameters.py b/core/common/swagger_parameters.py index bfbfbb28f..a46b70279 100644 --- a/core/common/swagger_parameters.py +++ b/core/common/swagger_parameters.py @@ -78,10 +78,12 @@ ) last_login_before_param = openapi.Parameter( - LAST_LOGIN_BEFORE_PARAM, openapi.IN_QUERY, description='YYYY-MM-DD (optional)', type=openapi.TYPE_STRING) + LAST_LOGIN_BEFORE_PARAM, openapi.IN_QUERY, description='YYYY-MM-DD (optional, staff only)', + type=openapi.TYPE_STRING) last_login_since_param = openapi.Parameter( - LAST_LOGIN_SINCE_PARAM, openapi.IN_QUERY, description='YYYY-MM-DD (optional)', type=openapi.TYPE_STRING) + LAST_LOGIN_SINCE_PARAM, openapi.IN_QUERY, description='YYYY-MM-DD (optional, staff only)', + type=openapi.TYPE_STRING) date_joined_before_param = openapi.Parameter( DATE_JOINED_BEFORE_PARAM, openapi.IN_QUERY, description='YYYY-MM-DD (optional)', type=openapi.TYPE_STRING) diff --git a/core/integration_tests/tests_users.py b/core/integration_tests/tests_users.py index 0e1b69d28..a3602fb8c 100644 --- a/core/integration_tests/tests_users.py +++ b/core/integration_tests/tests_users.py @@ -517,6 +517,29 @@ def test_get_staff_flag_filters_only_for_staff(self): self.assertEqual(response.status_code, 200) self.assertEqual([user['username'] for user in response.data], ['ocladmin']) + def test_get_last_login_filters_only_for_staff(self): + old_login_user = UserProfileFactory(username='oldloginuser', last_login='2020-01-01T00:00:00Z') + recent_login_user = UserProfileFactory(username='recentloginuser', last_login='2024-01-01T00:00:00Z') + all_users = ['ocladmin', 'oldloginuser', 'recentloginuser'] + + for headers in [{}, {'HTTP_AUTHORIZATION': 'Token ' + old_login_user.get_token()}]: + for filter_param in ['lastLoginSince=2022-01-01', 'lastLoginBefore=2022-01-01']: + response = self.client.get(f'/users/?{filter_param}', format='json', **headers) + + self.assertEqual(response.status_code, 200) + self.assertEqual(sorted(user['username'] for user in response.data), all_users) + + headers = {'HTTP_AUTHORIZATION': 'Token ' + self.superuser.get_token()} + response = self.client.get('/users/?lastLoginSince=2022-01-01', format='json', **headers) + + self.assertEqual(response.status_code, 200) + self.assertEqual([user['username'] for user in response.data], [recent_login_user.username]) + + response = self.client.get('/users/?lastLoginBefore=2022-01-01', format='json', **headers) + + self.assertEqual(response.status_code, 200) + self.assertEqual([user['username'] for user in response.data], [old_login_user.username]) + def test_get_verbose_plan_fields_only_for_self_and_staff(self): regular_user = UserProfileFactory(username='planfieldsregular') other_user = UserProfileFactory(username='planfieldsother') diff --git a/core/users/views.py b/core/users/views.py index a95eab19b..e4eb50c26 100644 --- a/core/users/views.py +++ b/core/users/views.py @@ -218,8 +218,10 @@ def get_faceted_filters(self, split=False, params=None, additional_fields=None, def get_queryset(self): updated_since = parse_updated_since_param(self.request.query_params) updated_by = self.request.query_params.get(UPDATED_BY_USERNAME_PARAM, None) - last_login_since = self.request.query_params.get(LAST_LOGIN_SINCE_PARAM, None) - last_login_before = self.request.query_params.get(LAST_LOGIN_BEFORE_PARAM, None) + last_login_since = last_login_before = None + if self.request.user.is_staff: # last login is staff-only account data + last_login_since = self.request.query_params.get(LAST_LOGIN_SINCE_PARAM, None) + last_login_before = self.request.query_params.get(LAST_LOGIN_BEFORE_PARAM, None) date_joined_since = self.request.query_params.get(DATE_JOINED_SINCE_PARAM, None) date_joined_before = self.request.query_params.get(DATE_JOINED_BEFORE_PARAM, None) if updated_since: From e1986bba62713f3a530addef85d401be14a07968 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:22:08 -0400 Subject: [PATCH 16/35] OpenConceptLab/ocl_online#224 | extras.* filters match only the named extras field The extras. filter value goes into an ES query_string, where a `field:value` clause can address any indexed field. Escape backslash and colon in the value so it only ever matches extras.. Co-Authored-By: Claude Opus 5.5 --- core/common/views.py | 3 ++- core/integration_tests/tests_users.py | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/core/common/views.py b/core/common/views.py index 779ae84d0..d5483f580 100644 --- a/core/common/views.py +++ b/core/common/views.py @@ -618,7 +618,8 @@ def get_extras_searchable_fields_from_query_params(self): for key, value in query_params.items(): if key.startswith('extras.') and not key.startswith('extras.exists') and not key.startswith('extras.exact'): parts = key.split('extras.') - value = value.replace('/', '\\/').replace('-', '_') + # escape `:` so the query_string value can only match this extras field + value = value.replace('\\', '\\\\').replace('/', '\\/').replace(':', '\\:').replace('-', '_') result['extras.' + parts[1].replace('.', '__')] = f"*{value}*" return result diff --git a/core/integration_tests/tests_users.py b/core/integration_tests/tests_users.py index a3602fb8c..743a1810f 100644 --- a/core/integration_tests/tests_users.py +++ b/core/integration_tests/tests_users.py @@ -517,6 +517,26 @@ def test_get_staff_flag_filters_only_for_staff(self): self.assertEqual(response.status_code, 200) self.assertEqual([user['username'] for user in response.data], ['ocladmin']) + def test_get_extras_filter_matches_only_the_extras_field(self): + regular_user = UserProfileFactory(username='extrasregularuser', extras={'xdept': 'xcardiology'}) + member = UserProfileFactory(username='extrasprivatemember') + private_org = OrganizationFactory(mnemonic='extrasprivateorg', public_access=ACCESS_TYPE_NONE) + private_org.members.add(member) + UserProfileDocument().update([regular_user, member]) + headers = {'HTTP_AUTHORIZATION': 'Token ' + regular_user.get_token()} + + response = self.client.get('/users/', {'extras.xdept': 'cardio'}, format='json', **headers) + + self.assertEqual(response.status_code, 200) + self.assertEqual([user['username'] for user in response.data], ['extrasregularuser']) + + for value in ['zzqq OR is_staff:true OR zzqq', 'zzqq OR org:extrasprivateorg OR zzqq', + 'zzqq OR is_staff\\:true OR zzqq']: + response = self.client.get('/users/', {'extras.xdept': value}, format='json', **headers) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data, []) + def test_get_last_login_filters_only_for_staff(self): old_login_user = UserProfileFactory(username='oldloginuser', last_login='2020-01-01T00:00:00Z') recent_login_user = UserProfileFactory(username='recentloginuser', last_login='2024-01-01T00:00:00Z') From 287593bffb3fb3066b23bcbf3a23c59b31dd4112 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:26:41 -0400 Subject: [PATCH 17/35] OpenConceptLab/ocl_online#224 | User detail: subscribed_orgs lists only orgs the requester can view; pins only for self and staff includeSubscribedOrgs now lists the user's public orgs plus the orgs the requester belongs to (all for staff), using the same rule as the /users//orgs/ list, now Organization.get_visible_to. includePins is returned only to the user themselves and staff, like the other private user fields. Co-Authored-By: Claude Opus 5.5 --- core/orgs/models.py | 13 +++++++++++++ core/orgs/views.py | 12 +----------- core/users/serializers.py | 11 +++++++---- core/users/tests/tests.py | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 15 deletions(-) diff --git a/core/orgs/models.py b/core/orgs/models.py index fb0898d00..92a59e02a 100644 --- a/core/orgs/models.py +++ b/core/orgs/models.py @@ -1,6 +1,7 @@ from django.contrib.contenttypes.fields import GenericRelation from django.core.validators import RegexValidator from django.db import models, transaction +from pydash import get from core.client_configs.models import ClientConfig from core.common.checksums import ChecksumModel @@ -79,6 +80,18 @@ def get_by_username(cls, username): def get_public(cls): return cls.objects.filter(public_access=ACCESS_TYPE_VIEW) + @classmethod + def get_visible_to(cls, user, queryset=None): + """Limits `queryset` (default: all orgs) to the orgs `user` can view: public orgs and the user's own. + Staff can view all.""" + queryset = cls.objects.all() if queryset is None else queryset + if get(user, 'is_superuser') or get(user, 'is_staff'): + return queryset + visible = cls.get_public() + if get(user, 'is_authenticated'): + visible = visible | cls.get_by_username(user.username) + return queryset.filter(id__in=visible.values('id')) + def save(self, *args, force_insert=False, force_update=False, using=None, update_fields=None): """ The creator becomes a member when the org is created. Later saves leave the members alone, so editing diff --git a/core/orgs/views.py b/core/orgs/views.py index e129a4eba..28175aab2 100644 --- a/core/orgs/views.py +++ b/core/orgs/views.py @@ -51,7 +51,7 @@ def get_queryset(self): username = get(self.request.user, 'username') if username: - self.queryset = self.get_visible(Organization.get_by_username(username)) + self.queryset = Organization.get_visible_to(self.request.user, Organization.get_by_username(username)) elif self.request.user.is_anonymous: self.queryset = Organization.get_public() elif self.request.user.is_superuser or self.request.user.is_staff: @@ -70,16 +70,6 @@ def get_queryset(self): return self.queryset.distinct() - def get_visible(self, queryset): - """Limits `queryset` to the orgs the requester can view: public orgs and the requester's own.""" - user = self.request.user - if user.is_superuser or user.is_staff: - return queryset - visible = Organization.get_public() - if user.is_authenticated: - visible = visible | Organization.get_by_username(user.username) - return queryset.filter(id__in=visible.values('id')) - def get_serializer_class(self): if self.request.method == 'GET' and self.is_verbose(): return OrganizationDetailSerializer diff --git a/core/users/serializers.py b/core/users/serializers.py index 83581e93d..0f40f1462 100644 --- a/core/users/serializers.py +++ b/core/users/serializers.py @@ -18,7 +18,7 @@ TRUTHY = get_truthy_values() PRIVATE_USER_FIELDS = ( - 'email', 'last_login', 'is_staff', 'is_superuser', 'auth_groups', 'permissions', 'capabilities') + 'email', 'last_login', 'is_staff', 'is_superuser', 'auth_groups', 'permissions', 'capabilities', 'pins') def can_view_private_user_fields(request, user): @@ -30,7 +30,7 @@ def can_view_private_user_fields(request, user): class PrivateUserFieldsMixin: - """Email, account flags and plan fields are only for the user themselves and staff.""" + """Email, account flags, plan fields and pins are only for the user themselves and staff.""" def to_representation(self, instance): data = super().to_representation(instance) if not can_view_private_user_fields(get(self, 'context.request'), instance): @@ -245,9 +245,12 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def get_subscribed_orgs(self, obj): + """The user's orgs that the requester can view.""" if self.include_subscribed_orgs: + from core.orgs.models import Organization from core.orgs.serializers import OrganizationListSerializer - return OrganizationListSerializer(obj.organizations.all(), many=True).data + orgs = Organization.get_visible_to(get(self, 'context.request.user'), obj.organizations.all()) + return OrganizationListSerializer(orgs, many=True).data return None @@ -267,7 +270,7 @@ def get_capabilities(self, obj): return CapabilitySerializer(obj.capabilities, many=True, context={'user': obj}).data def get_pins(self, obj): - if self.include_pins: + if self.include_pins and can_view_private_user_fields(get(self, 'context.request'), obj): from core.pins.serializers import PinSerializer return PinSerializer(obj.pins.prefetch_related('resource').all(), many=True).data diff --git a/core/users/tests/tests.py b/core/users/tests/tests.py index 4ef2fad5d..1be213874 100644 --- a/core/users/tests/tests.py +++ b/core/users/tests/tests.py @@ -810,6 +810,41 @@ def test_user_detail_plan_fields_only_for_self_and_staff(self): self.assertIn( {'name': 'mapper.match_operations', 'limit': 100, 'used': 0}, response.data['capabilities']) + def test_user_subscribed_orgs_and_pins_only_as_visible_to_requester(self): + public_org = OrganizationFactory(mnemonic='visorgspublic') + private_org = OrganizationFactory(mnemonic='visorgsprivate', public_access=ACCESS_TYPE_NONE) + user = UserProfileFactory(username='visorgsuser', organizations=[public_org, private_org]) + co_member = UserProfileFactory(username='visorgscomember', organizations=[private_org]) + other_user = UserProfileFactory(username='visorgsother') + private_source = OrganizationSourceFactory(organization=private_org, public_access=ACCESS_TYPE_NONE) + user.pins.create(resource=private_source) + params = 'includeSubscribedOrgs=true&includePins=true' + url = f'/users/{user.username}/?{params}' + + for requester, orgs in [(other_user, ['visorgspublic']), (co_member, ['visorgsprivate', 'visorgspublic'])]: + response = self.client.get(url, HTTP_AUTHORIZATION='Token ' + requester.get_token()) + + self.assertEqual(response.status_code, 200) + self.assertEqual(sorted(org['id'] for org in response.data['subscribed_orgs']), orgs) + self.assertNotIn('pins', response.data) + + for token in [user.get_token(), self.admin_token]: + response = self.client.get(url, HTTP_AUTHORIZATION='Token ' + token) + + self.assertEqual(response.status_code, 200) + self.assertEqual( + sorted(org['id'] for org in response.data['subscribed_orgs']), ['visorgsprivate', 'visorgspublic']) + self.assertEqual([pin['resource_uri'] for pin in response.data['pins']], [private_source.uri]) + + response = self.client.get(f'/users/?verbose=true&{params}') + + self.assertEqual(response.status_code, 200) + users = {user['username']: user for user in response.data} + self.assertEqual([org['id'] for org in users[user.username]['subscribed_orgs']], ['visorgspublic']) + self.assertEqual([org['id'] for org in users[co_member.username]['subscribed_orgs']], []) + for user_data in users.values(): + self.assertNotIn('pins', user_data) + @patch('core.users.models.UserProfile.get_all_permissions', return_value=set()) @patch('core.users.models.UserProfile.get_capability_usage', return_value=0) def test_user_detail_does_not_compute_plan_for_other_users(self, usage_mock, permissions_mock): From 5f6b1d843ee778f81add1e87b453e8e92356d99f Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:29:22 -0400 Subject: [PATCH 18/35] OpenConceptLab/ocl_online#224 | Pins: list and read only the pins whose org and resource the requester can view GET /users//pins/, /orgs//pins/ and the pin detail return only pins whose pinned source, collection or org the requester can view, and, for org pins, whose org they can view (the same check as pinning). The owner can still change or remove every pin. includePins on the user detail applies the same rule. Co-Authored-By: Claude Opus 5.5 --- core/pins/models.py | 12 +++++++++++ core/pins/permissions.py | 4 ++-- core/pins/tests.py | 45 +++++++++++++++++++++++++++++++++++++++ core/pins/views.py | 7 ++++-- core/users/serializers.py | 7 ++++-- 5 files changed, 69 insertions(+), 6 deletions(-) diff --git a/core/pins/models.py b/core/pins/models.py index 742e97094..de0387178 100644 --- a/core/pins/models.py +++ b/core/pins/models.py @@ -73,6 +73,18 @@ def get_resource(cls, resource_type, resource_id): return None + @staticmethod + def get_visible_to(queryset, request): + """Limits `queryset` to the pins whose org (for org pins) and pinned resource the requester can view.""" + from core.common.permissions import CanViewConceptDictionary + permission = CanViewConceptDictionary() + + def can_view(obj): # a user pin has no org; a pin whose resource is gone shows no resource + return obj is None or permission.has_object_permission(request, None, obj) + + return queryset.filter( + id__in=[pin.id for pin in queryset if can_view(pin.organization) and can_view(pin.resource)]) + @property def parent(self): if self.organization_id: diff --git a/core/pins/permissions.py b/core/pins/permissions.py index 01788010f..a6db981f4 100644 --- a/core/pins/permissions.py +++ b/core/pins/permissions.py @@ -5,8 +5,8 @@ class CanEditPins(BasePermission): """ - Anyone can read pins. 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. + Anyone can read the pins they can view (Pin.get_visible_to). 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): if request.method in SAFE_METHODS: diff --git a/core/pins/tests.py b/core/pins/tests.py index 6f86b7b7a..29565133c 100644 --- a/core/pins/tests.py +++ b/core/pins/tests.py @@ -2,12 +2,14 @@ from mock import Mock from core.collections.tests.factories import OrganizationCollectionFactory +from core.common.constants import ACCESS_TYPE_NONE from core.common.tests import OCLTestCase, OCLAPITestCase from core.orgs.models import Organization from core.orgs.tests.factories import OrganizationFactory from core.pins.models import Pin from core.sources.models import Source from core.sources.tests.factories import OrganizationSourceFactory +from core.users.models import UserProfile from core.users.tests.factories import UserProfileFactory @@ -222,6 +224,49 @@ def test_get_200_with_creator_pins(self): sorted([source.uri, collection.uri]) ) + def test_get_200_only_pins_the_requester_can_view(self): + private_org = OrganizationFactory(public_access=ACCESS_TYPE_NONE) + public_org = OrganizationFactory() + owner = UserProfileFactory(organizations=[private_org, public_org]) + private_source = OrganizationSourceFactory(organization=private_org, public_access=ACCESS_TYPE_NONE) + public_source = OrganizationSourceFactory(organization=public_org) + private_user_pin = owner.pins.create(resource=private_source) + owner.pins.create(resource=public_source) + owner.pins.create(resource=private_org) + public_org.pins.create(resource=private_source) + public_org.pins.create(resource=public_source) + private_org.pins.create(resource=public_source) + admin = UserProfile.objects.get(username='ocladmin') + + def get_pin_uris(url, token=None): + headers = {'HTTP_AUTHORIZATION': 'Token ' + token} if token else {} + response = self.client.get(url, format='json', **headers) + 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]) + self.assertEqual(get_pin_uris(public_org.uri + 'pins/', token), [public_source.uri]) + self.assertEqual(get_pin_uris(private_org.uri + 'pins/', token), []) + 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 token in [owner.get_token(), admin.get_token()]: + self.assertEqual( + get_pin_uris(owner.uri + 'pins/', token), + sorted([private_source.uri, public_source.uri, private_org.uri])) + self.assertEqual( + get_pin_uris(public_org.uri + 'pins/', token), sorted([private_source.uri, public_source.uri])) + self.assertEqual(get_pin_uris(private_org.uri + 'pins/', token), [public_source.uri]) + + private_org.members.remove(owner) + self.assertEqual(get_pin_uris(owner.uri + 'pins/', owner.get_token()), [public_source.uri]) + response = self.client.get( + owner.uri + '?includePins=true', HTTP_AUTHORIZATION='Token ' + owner.get_token(), format='json') + self.assertEqual([pin['resource_uri'] for pin in response.data['pins']], [public_source.uri]) + def test_post_201(self): source = OrganizationSourceFactory() org = OrganizationFactory() diff --git a/core/pins/views.py b/core/pins/views.py index f4c8edd19..2f2662d58 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 @@ -60,9 +60,12 @@ def get_queryset(self): criteria = Q(**filters) if self.should_include_creator_pins() and 'org' not in self.kwargs: criteria |= Q(created_by_id=self.request.user.id, user_id__isnull=True) - return Pin.objects.filter( + queryset = Pin.objects.filter( criteria ).select_related('organization', 'user').prefetch_related('resource') + if self.request.method in SAFE_METHODS: + queryset = Pin.get_visible_to(queryset, self.request) + return queryset class PinListView(PinBaseView, ListAPIView): diff --git a/core/users/serializers.py b/core/users/serializers.py index 0f40f1462..d42e81468 100644 --- a/core/users/serializers.py +++ b/core/users/serializers.py @@ -270,9 +270,12 @@ def get_capabilities(self, obj): return CapabilitySerializer(obj.capabilities, many=True, context={'user': obj}).data def get_pins(self, obj): - if self.include_pins and can_view_private_user_fields(get(self, 'context.request'), obj): + request = get(self, 'context.request') + if self.include_pins and can_view_private_user_fields(request, obj): + from core.pins.models import Pin from core.pins.serializers import PinSerializer - return PinSerializer(obj.pins.prefetch_related('resource').all(), many=True).data + return PinSerializer( + Pin.get_visible_to(obj.pins.prefetch_related('resource').all(), request), many=True).data return None From 85de1b0ea349e6e4d89f59b94d1d954d3f2a79ec Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:38:34 -0400 Subject: [PATCH 19/35] OpenConceptLab/ocl_online#224 | CSV exports: build each export for its requester and store it under that requester ?csv=true no longer reuses an existing export file looked up by URL path alone. Every export is built from the view's own queryset, so the view's access checks and visibility filters apply, and is written to a downloads folder of the requester's own (user id, or "anonymous"), so one requester's file is never served to another. The ?csv&type= path uses the same rule. Removes the unused export-reuse helpers. Co-Authored-By: Claude Opus 5.5 --- core/common/mixins.py | 39 +++------- core/common/utils.py | 19 +---- .../integration_tests/tests_access_control.py | 73 ++++++++++++++++++- 3 files changed, 85 insertions(+), 46 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index b8e684974..991b386dd 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -30,7 +30,7 @@ from core.users.constants import LIST_UNPAGINATED_PERMISSION from .checksums import ChecksumModel from .exceptions import Http403, UnpaginatedListLimitReached -from .utils import write_csv_to_s3, get_csv_from_s3, get_query_params_from_url_string, compact_dict_by_values, \ +from .utils import write_csv_to_s3, get_query_params_from_url_string, compact_dict_by_values, \ to_owner_uri, parse_updated_since_param, get_export_service, to_int, get_truthy_values, generate_temp_version, \ canonical_url_to_url_and_version, decode_string, to_parent_kwargs_from_uri from ..concepts.constants import PERSIST_CLONE_ERROR @@ -333,15 +333,10 @@ def get_object_ids(self): return map(lambda o: o.id, self.object_list[0:100]) def get_csv(self, request, queryset=None): - filename, url, prepare_new_file, is_member = None, None, True, False - - parent = None # TODO: fix this for parent (owner) - - if parent: - prepare_new_file = False - user = request.query_params.get('user', None) - is_member = self._is_member(parent, user) - + """ + Builds the CSV from the view's own queryset, so the view's access checks apply, and stores it in a folder + of the requester's own: one requester's export is never served to another. + """ try: path = request.__dict__.get('_request').path filename = '_'.join(compact(path.split('/'))).replace('.', '_') @@ -351,28 +346,14 @@ def get_csv(self, request, queryset=None): except Exception: # pylint: disable=broad-except kwargs = {} - if filename and prepare_new_file: - url = get_csv_from_s3(filename, is_member) - - if not url: - queryset = queryset or self._get_query_set_from_view(is_member) - data = self.get_csv_rows(queryset) if hasattr(self, 'get_csv_rows') else queryset.values() - url = write_csv_to_s3(data, is_member, **kwargs) + if queryset is None: + queryset = self.get_queryset()[0:CSV_DEFAULT_LIMIT] + data = self.get_csv_rows(queryset) if hasattr(self, 'get_csv_rows') else queryset.values() + requester = request.user.id if request.user.is_authenticated else 'anonymous' + url = write_csv_to_s3(data, requester, **kwargs) return Response({'url': url}, status=200) - @staticmethod - def _is_member(parent, requesting_user): - if not parent or type(parent).__name__ in ['UserProfile', 'Organization']: - return False - - owner = parent.owner - return owner.members.filter(username=requesting_user).exists() if type(owner).__name__ == 'Organization' else \ - requesting_user == parent.created_by - - def _get_query_set_from_view(self, is_member): - return self.get_queryset() if is_member else self.get_queryset()[0:CSV_DEFAULT_LIMIT] - class PathWalkerMixin: """ diff --git a/core/common/utils.py b/core/common/utils.py index 097b74dd3..5ebdce68e 100644 --- a/core/common/utils.py +++ b/core/common/utils.py @@ -43,7 +43,8 @@ def cd_temp(): return cwd -def write_csv_to_s3(data, is_owner, **kwargs): # pragma: no cover +def write_csv_to_s3(data, requester, **kwargs): # pragma: no cover + """Uploads `data` as a zipped CSV to the requester's own downloads folder and returns its URL.""" cwd = cd_temp() csv_file = csv_file_for(data, **kwargs) csv_file.close() @@ -51,7 +52,7 @@ def write_csv_to_s3(data, is_owner, **kwargs): # pragma: no cover with zipfile.ZipFile(zip_file_name, 'w', zipfile.ZIP_DEFLATED) as zip_file: zip_file.write(csv_file.name) - key = get_downloads_path(is_owner) + zip_file.filename + key = f'downloads/reader/{requester}/' + zip_file.filename get_export_service().upload_file( key=key, file_path=os.path.abspath(zip_file.filename), binary=True, metadata={'ContentType': 'application/zip'}, headers={'content-type': 'application/zip'} @@ -69,20 +70,6 @@ def compact_dict_by_values(_dict): return copied_dict -def get_downloads_path(is_owner): # pragma: no cover - return 'downloads/creator/' if is_owner else 'downloads/reader/' - - -def get_csv_from_s3(filename, is_owner): # pragma: no cover - filename = get_downloads_path(is_owner) + filename + '.csv.zip' - - export_service = get_export_service() - if export_service.exists(filename): - return export_service.url_for(filename) - - return None - - def reverse_resource(resource, viewname, args=None, kwargs=None, **extra): """ Generate the URL for the view specified as viewname of the object specified as resource. diff --git a/core/integration_tests/tests_access_control.py b/core/integration_tests/tests_access_control.py index 4d7781f27..84d0677ad 100644 --- a/core/integration_tests/tests_access_control.py +++ b/core/integration_tests/tests_access_control.py @@ -1,3 +1,7 @@ +import csv +import io +import zipfile + from mock import patch, Mock from core.client_configs.models import ClientConfig @@ -9,13 +13,14 @@ from core.common.tests import OCLAPITestCase, OCLTestCase from core.concepts.models import Concept from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory, ConceptDescriptionFactory -from core.map_projects.tests.factories import MapProjectFactory +from core.map_projects.tests.factories import MapProjectFactory, AutomatchRunFactory from core.orgs.models import Organization from core.orgs.tests.factories import OrganizationFactory from core.pins.models import Pin from core.sources.documents import SourceDocument from core.sources.models import Source from core.sources.tests.factories import OrganizationSourceFactory, UserSourceFactory +from core.tasks.models import Task from core.url_registry.factories import GlobalURLRegistryFactory, OrganizationURLRegistryFactory, UserURLRegistryFactory from core.url_registry.models import URLRegistry from core.users.models import UserProfile @@ -553,6 +558,72 @@ def test_members_and_staff_see_private_orgs(self): self.list_orgs(self.member.uri + 'orgs/', self.other_member), all_orgs) +class CsvExportAccessTest(AccessTestMixin, OCLAPITestCase): + """A ?csv=true export holds only rows the requester can list, and one requester's export is never served to + another.""" + def setUp(self): + super().setUp() + self.exports = {} + export_service = Mock( + upload_file=self.upload, exists=lambda key: key in self.exports, + url_for=lambda key: 'https://exports.test/' + key) + patcher = patch('core.common.utils.get_export_service', return_value=export_service) + patcher.start() + self.addCleanup(patcher.stop) + + def upload(self, key, file_path=None, **kwargs): # pylint: disable=unused-argument + with open(file_path, 'rb') as file: + self.exports[key] = file.read() + + def export(self, url, user, column): + """The `column` values of the CSV export at `url` for `user`.""" + response = self.request('get', url, user) + self.assertEqual(response.status_code, 200) + 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 sorted(row[column] for row in csv.DictReader(io.TextIOWrapper(csv_file, 'utf-8-sig'))) + + def test_org_list(self): + private_org = OrganizationFactory(public_access=ACCESS_TYPE_NONE) + + self.assertIn(private_org.mnemonic, self.export('/orgs/?csv=true', self.staff, 'mnemonic')) + for url in ['/orgs/?csv=true', '/orgs/?csv=true&type=Organization']: + mnemonics = self.export(url, None, 'mnemonic') + self.assertIn(self.org.mnemonic, mnemonics) + self.assertNotIn(private_org.mnemonic, mnemonics) + + def test_task_list(self): + Task.objects.create(name='csv-owner-task', created_by=self.owner) + url = self.owner.uri + 'tasks/?csv=true' + + self.assertEqual(self.export(url, self.owner, 'name'), ['csv-owner-task']) + self.assertEqual(self.export(url, self.outsider, 'name'), []) + + 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' + + self.assertEqual(self.export(url, self.owner, 'name'), ['csv-project']) + self.assertEqual(self.export(url, None, 'name'), []) + + def test_repo_concepts(self): + source = UserSourceFactory(user=self.owner, public_access=ACCESS_TYPE_NONE) + concept = ConceptFactory(parent=source, mnemonic='csv-concept') + url = source.concepts_url + '?csv=true&limit=1000' + + self.assertEqual(self.export(url, self.owner, 'mnemonic'), [concept.mnemonic]) + self.assertEqual(self.request('get', url, self.outsider).status_code, 403) + + def test_auto_match_run_list_checks_the_project(self): + project = MapProjectFactory(user=self.owner, organization=None, public_access=ACCESS_TYPE_NONE) + run = AutomatchRunFactory(map_project=project) + url = f'{self.owner.uri}map-projects/{project.id}/auto-match-runs/?csv=true' + + self.assertEqual(self.export(url, self.owner, 'id'), [str(run.id)]) + self.assertEqual(self.request('get', url, self.outsider).status_code, 403) + self.assertEqual(self.request('get', url, None).status_code, 401) + + class OrganizationExtrasAccessTest(AccessTestMixin, OCLAPITestCase): def setUp(self): super().setUp() From 59fcc6dfb434b676f55b8d6532d33280c4fa1f69 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:59:13 -0400 Subject: [PATCH 20/35] OpenConceptLab/ocl_online#224 | Pin updates use the same visibility filter as reads An update returns the pinned resource, so updating a pin whose org or resource the requester can't view is refused like reading it (404). The pin's owner can still delete it. Co-Authored-By: Claude Opus 5.5 --- core/pins/tests.py | 8 ++++++++ core/pins/views.py | 5 +++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/core/pins/tests.py b/core/pins/tests.py index 29565133c..b8d9610b5 100644 --- a/core/pins/tests.py +++ b/core/pins/tests.py @@ -267,6 +267,14 @@ def get_pin_uris(url, token=None): owner.uri + '?includePins=true', HTTP_AUTHORIZATION='Token ' + owner.get_token(), format='json') self.assertEqual([pin['resource_uri'] for pin in response.data['pins']], [public_source.uri]) + # an update would return the resource, so it's refused; the owner can still delete the pin + pin_url = owner.uri + f'pins/{private_user_pin.id}/' + response = self.client.patch( + pin_url, {'order': 0}, HTTP_AUTHORIZATION='Token ' + owner.get_token(), format='json') + self.assertEqual(response.status_code, 404) + response = self.client.delete(pin_url, HTTP_AUTHORIZATION='Token ' + owner.get_token(), format='json') + self.assertEqual(response.status_code, 204) + def test_post_201(self): source = OrganizationSourceFactory() org = OrganizationFactory() diff --git a/core/pins/views.py b/core/pins/views.py index 2f2662d58..620da2a8d 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, SAFE_METHODS +from rest_framework.permissions import IsAuthenticatedOrReadOnly from rest_framework.response import Response from core.common.constants import MAX_PINS_ALLOWED, INCLUDE_CREATOR_PINS @@ -63,7 +63,8 @@ def get_queryset(self): queryset = Pin.objects.filter( criteria ).select_related('organization', 'user').prefetch_related('resource') - if self.request.method in SAFE_METHODS: + # an update returns the pinned resource, so only a pin's owner deleting it skips the visibility filter + if self.request.method != 'DELETE': queryset = Pin.get_visible_to(queryset, self.request) return queryset From a359a086fda2ea0e2040f929f6ea2dc9fac913a6 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:59:13 -0400 Subject: [PATCH 21/35] OpenConceptLab/ocl_online#224 | events/0011: a followed repo version is judged by its own access A Followed/Unfollowed event of a repo version that isn't public is made private too, even when its HEAD is public; anything else under a repo is still judged by the HEAD. Co-Authored-By: Claude Opus 5.5 --- core/events/migrations/0011_private_follow_events.py | 6 ++++-- core/events/tests/tests.py | 10 +++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/core/events/migrations/0011_private_follow_events.py b/core/events/migrations/0011_private_follow_events.py index c5866d485..d99952073 100644 --- a/core/events/migrations/0011_private_follow_events.py +++ b/core/events/migrations/0011_private_follow_events.py @@ -21,8 +21,10 @@ def make_follow_events_of_private_objects_private(apps, schema_editor): # pylin private_uris = get_private_uris(apps.get_model('orgs', 'Organization'), urls) for app_label, model_name in [('sources', 'Source'), ('collections', 'Collection')]: - private_uris |= get_private_uris( - apps.get_model(app_label, model_name), set(repo_uris.values()), version='HEAD') + model = apps.get_model(app_label, model_name) + # a followed repo version by its own access, anything else under a repo by its HEAD's + private_uris |= get_private_uris(model, urls) + private_uris |= get_private_uris(model, set(repo_uris.values()), version='HEAD') private_urls = [url for url in urls if url in private_uris or repo_uris.get(url) in private_uris] if private_urls: diff --git a/core/events/tests/tests.py b/core/events/tests/tests.py index f182b1bab..3b4ed7015 100644 --- a/core/events/tests/tests.py +++ b/core/events/tests/tests.py @@ -115,7 +115,7 @@ def test_clean_fields(self): class PrivateFollowEventsMigrationTest(OCLTestCase): - def test_migration_makes_follow_events_of_private_objects_private(self): + def test_migration_makes_follow_events_of_private_objects_private(self): # pylint: disable=too-many-locals migration = importlib.import_module('core.events.migrations.0011_private_follow_events') follower = UserProfileFactory() private_org = OrganizationFactory(public_access='None') @@ -124,9 +124,13 @@ def test_migration_makes_follow_events_of_private_objects_private(self): public_source = OrganizationSourceFactory(organization=private_org) private_concept = ConceptFactory(parent=private_source) public_concept = ConceptFactory(parent=public_source) + private_version = OrganizationSourceFactory( + organization=public_source.organization, mnemonic=public_source.mnemonic, version='v1', + public_access='None') + private_version.refresh_from_db() urls = [ follower.url, private_org.url, public_org.url, private_source.url, public_source.url, - private_concept.url, public_concept.url, '/orgs/deleted-org/' + private_concept.url, public_concept.url, '/orgs/deleted-org/', private_version.uri ] events = { (event_type, url): Event.objects.create( @@ -136,7 +140,7 @@ def test_migration_makes_follow_events_of_private_objects_private(self): migration.make_follow_events_of_private_objects_private(apps, None) - private_urls = [private_org.url, private_source.url, private_concept.url] + private_urls = [private_org.url, private_source.url, private_concept.url, private_version.uri] for (event_type, url), event in events.items(): event.refresh_from_db() self.assertEqual(event.public, event_type == 'Created' or url not in private_urls, (event_type, url)) From 48646838a250c7bab8737a20ae3eee540075db8e Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:59:13 -0400 Subject: [PATCH 22/35] OpenConceptLab/ocl_online#224 | CodeSystem $validate-code: a url the requester can't view gives false, with or without a code Co-Authored-By: Claude Opus 5.5 --- core/code_systems/tests.py | 9 ++++++++- core/code_systems/views.py | 2 ++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/core/code_systems/tests.py b/core/code_systems/tests.py index a2d3e52c3..ff5384f11 100644 --- a/core/code_systems/tests.py +++ b/core/code_systems/tests.py @@ -262,7 +262,7 @@ def test_lookup_for_code_system_with_new_canonical(self): @patch('core.sources.models.index_source_concepts', Mock(__name__='index_source_concepts')) @patch('core.sources.models.index_source_mappings', Mock(__name__='index_source_mappings')) - def test_private_code_system_answers_as_missing_for_non_viewers(self): + def test_private_code_system_answers_as_missing_for_non_viewers(self): # pylint: disable=too-many-locals private_org = OrganizationFactory() private_source = OrganizationSourceFactory( organization=private_org, canonical_url='/private/url', public_access=ACCESS_TYPE_NONE) @@ -278,7 +278,14 @@ def test_private_code_system_answers_as_missing_for_non_viewers(self): not_found = self.client.get(lookup_url.format('/unknown/url', concept.mnemonic)) self.assertEqual(not_found.status_code, 400) + public_concept = ConceptFactory() + selector = (f'&org={public_concept.parent.organization.mnemonic}&source={public_concept.parent.mnemonic}' + f'&concept={public_concept.mnemonic}') for headers in [{}, outsider]: + # without a code, a code system the requester can't view still answers as one that doesn't exist + for system_url in ['/private/url', '/unknown/url']: + response = self.client.get(f'/fhir/CodeSystem/$validate-code/?url={system_url}{selector}', **headers) + self.assertFalse(response.data['parameter'][0]['valueBoolean'], (system_url, headers)) for url in [validate_url, f'{validate_url}&display={concept.display_name}']: response = self.client.get(url, **headers) self.assertEqual(response.status_code, 200, (url, headers)) diff --git a/core/code_systems/views.py b/core/code_systems/views.py index 53ebd3187..1bda2d71b 100644 --- a/core/code_systems/views.py +++ b/core/code_systems/views.py @@ -173,6 +173,8 @@ def get_queryset(self): else: source = source.filter(is_latest_version=True).exclude(version=HEAD) source = get_viewable_repo(source, self.request.user) + if not source: + return queryset.none() if code: if not source: From fe5badca99d5b39813964593a1c22b890a76331d Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 00:59:13 -0400 Subject: [PATCH 23/35] OpenConceptLab/ocl_online#224 | $cascade entries list the collection versions their requester can view A $cascade bundle serializes its entries without the request, so it now passes its requester, and verbose mapping entries show members and staff the private collection versions they can view. Co-Authored-By: Claude Opus 5.5 --- core/bundles/models.py | 5 +++-- core/concepts/serializers.py | 6 ++++-- core/integration_tests/tests_public_read_access.py | 4 ++++ core/mappings/serializers.py | 6 ++++-- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/core/bundles/models.py b/core/bundles/models.py index f7dc8952d..46f5f51d9 100644 --- a/core/bundles/models.py +++ b/core/bundles/models.py @@ -192,8 +192,9 @@ def cascade_as_hierarchy(self): self.root, context={'request': {'query_params': self.params}}).data def set_entries(self): - self.entries += self.get_concept_serializer()(self.concepts, many=True).data - self.entries += self.get_mapping_serializer()(self.mappings, many=True).data + context = {'user': self.user} + self.entries += self.get_concept_serializer()(self.concepts, many=True, context=context).data + self.entries += self.get_mapping_serializer()(self.mappings, many=True, context=context).data def get_mapping_serializer(self): from core.mappings.models import Mapping diff --git a/core/concepts/serializers.py b/core/concepts/serializers.py index fb6fa2785..b199a069f 100644 --- a/core/concepts/serializers.py +++ b/core/concepts/serializers.py @@ -363,7 +363,8 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def get_collection_versions(self, obj): - return obj.get_collection_versions(get(self, 'context.request.user')) + # a $cascade bundle passes its requester as `user` + return obj.get_collection_versions(get(self, 'context.request.user') or get(self, 'context.user')) class ConceptVersionCascadeSerializer(ConceptVersionListSerializer): @@ -693,7 +694,8 @@ def get_parent_concepts(self, obj): return None def get_collection_versions(self, obj): - return obj.get_collection_versions(get(self, 'context.request.user')) + # a $cascade bundle passes its requester as `user` + return obj.get_collection_versions(get(self, 'context.request.user') or get(self, 'context.user')) class ConceptHierarchySerializer(ModelSerializer): diff --git a/core/integration_tests/tests_public_read_access.py b/core/integration_tests/tests_public_read_access.py index 60b54ab2c..222f7051d 100644 --- a/core/integration_tests/tests_public_read_access.py +++ b/core/integration_tests/tests_public_read_access.py @@ -98,6 +98,10 @@ def test_private_collection_versions_only_for_members_and_staff(self): sorted(data['version_url'] for data in self.get(resource.uri + 'collection-versions/', user)), expected, (user, resource)) + entries = self.get(self.public_concept.uri + '$cascade/?verbose=true', user)['entry'] + self.assertEqual( + [entry['collection_versions'] for entry in entries if entry['type'] == 'Mapping'], [expected], user) + class FeedAccessTest(PrivateRepoReadAccessBaseTest): def test_source_feed_only_for_public_sources(self): diff --git a/core/mappings/serializers.py b/core/mappings/serializers.py index 8dde4783f..291fa3de2 100644 --- a/core/mappings/serializers.py +++ b/core/mappings/serializers.py @@ -163,7 +163,8 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def get_collection_versions(self, obj): - return obj.get_collection_versions(get(self, 'context.request.user')) + # a $cascade bundle passes its requester as `user` + return obj.get_collection_versions(get(self, 'context.request.user') or get(self, 'context.user')) class MappingChecksumSerializer(AbstractResourceSerializer): @@ -330,4 +331,5 @@ class Meta: ) def get_collection_versions(self, obj): - return obj.get_collection_versions(get(self, 'context.request.user')) + # a $cascade bundle passes its requester as `user` + return obj.get_collection_versions(get(self, 'context.request.user') or get(self, 'context.user')) From 5be274f1df5b0c817cf1cfc2e7851b015939c4f7 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 01:07:30 -0400 Subject: [PATCH 24/35] OpenConceptLab/ocl_online#224 | A user's own following list also hides objects they can no longer view Only staff get every follow; the user themselves sees a follow only while they can view the followed object, like everyone else. Co-Authored-By: Claude Opus 5.5 --- core/integration_tests/tests_users.py | 6 ++++++ core/users/models.py | 5 +++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/core/integration_tests/tests_users.py b/core/integration_tests/tests_users.py index 743a1810f..e12f56858 100644 --- a/core/integration_tests/tests_users.py +++ b/core/integration_tests/tests_users.py @@ -1346,6 +1346,12 @@ 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 + 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, staff), all_urls) + def test_post(self): follower = UserProfileFactory(username='follower') followed = UserProfileFactory(username='followed') diff --git a/core/users/models.py b/core/users/models.py index 94b592443..f2e729ac0 100644 --- a/core/users/models.py +++ b/core/users/models.py @@ -439,9 +439,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 and staff get them all.""" + """This user's follows, without those of objects `viewer` can't view (the user too, once they lose access). + Staff get them all.""" follows = self.following.prefetch_related('following') - if getattr(viewer, 'is_staff', False) or getattr(viewer, 'id', None) == self.id: + if getattr(viewer, 'is_staff', False): 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 From d1a4b834ceceadfcd77af72dcd154f221cfe569e Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 01:07:30 -0400 Subject: [PATCH 25/35] OpenConceptLab/ocl_online#224 | Events show their objects only to requesters who can view them now An event's public flag is set when it's recorded, but the objects it shows are serialized live. An object the requester can't view now (for example a repo made private after the event) is shown as null. Co-Authored-By: Claude Opus 5.5 --- core/events/serializers.py | 17 +++++++++++------ core/integration_tests/tests_events.py | 21 +++++++++++++++++++++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/core/events/serializers.py b/core/events/serializers.py index 257a05a6d..c93cce560 100644 --- a/core/events/serializers.py +++ b/core/events/serializers.py @@ -1,3 +1,4 @@ +from pydash import get from rest_framework import serializers from core.events.models import Event @@ -16,19 +17,23 @@ class Meta: 'actor', 'public', 'url', 'created_at' ) - @staticmethod - def get_referenced_object(obj): + def can_view(self, instance): + """An event's flag is set when it's recorded; the object shown is live, so its access is checked now.""" + from core.users.models import UserProfile + return UserProfile.can_user_view(get(self, 'context.request.user'), instance) + + def get_referenced_object(self, obj): referenced_object = obj.referenced_object if referenced_object: - data = referenced_object.get_brief_serializer()(referenced_object).data + data = referenced_object.get_brief_serializer()(referenced_object).data if self.can_view( + referenced_object) else None else: data = obj._referenced_object return data - @staticmethod - def get_object(obj): + def get_object(self, obj): resource_object = obj.object data = None - if resource_object: + if resource_object and self.can_view(resource_object): data = resource_object.get_brief_serializer()(resource_object).data return data diff --git a/core/integration_tests/tests_events.py b/core/integration_tests/tests_events.py index 7a5d03d41..8da704fc2 100644 --- a/core/integration_tests/tests_events.py +++ b/core/integration_tests/tests_events.py @@ -278,6 +278,27 @@ 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): + org = OrganizationFactory(mnemonic='turned-private-org') + source = OrganizationSourceFactory(organization=org, mnemonic='turned-private') + member = UserProfileFactory(username='turned-member') + org.members.add(member) + follower = UserProfileFactory(username='turned-follower') + follower.follow(source) + source.public_access = 'None' + source.save() + + def get_referenced_objects(user=None): + headers = {'HTTP_AUTHORIZATION': f'Token {user.get_token()}'} if user else {} + response = self.client.get('/users/turned-follower/events/', **headers) + self.assertEqual(response.status_code, 200) + return [event['referenced_object'] 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 + for user in [None, follower]: + self.assertEqual(get_referenced_objects(user), [None], user) + self.assertEqual([obj['url'] for obj in get_referenced_objects(member)], [source.url]) + 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') From 1fae4041ad116965578da84d797f32f35a6a1f94 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 01:24:38 -0400 Subject: [PATCH 26/35] OpenConceptLab/ocl_online#224 | /parents/ and /children/ aren't searchable A search on them took the list search path, which lists the repo's concepts, not the concept's parents or children, and skips the view's own checks. They now always list the concept's hierarchy. Co-Authored-By: Claude Opus 5.5 --- core/concepts/views.py | 2 ++ core/integration_tests/tests_cross_repo_access.py | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/core/concepts/views.py b/core/concepts/views.py index 910a6725b..667b30592 100644 --- a/core/concepts/views.py +++ b/core/concepts/views.py @@ -595,6 +595,7 @@ def get_clone_to_source(self): class ConceptChildrenView(ConceptBaseView, ListWithHeadersMixin): + is_searchable = False # a search would list the repo's concepts, not this concept's hierarchy serializer_class = ConceptChildrenSerializer default_qs_sort_attr = 'mnemonic' @@ -614,6 +615,7 @@ def get(self, request, *args, **kwargs): class ConceptParentsView(ConceptBaseView, ListWithHeadersMixin): + is_searchable = False # a search would list the repo's concepts, not this concept's hierarchy serializer_class = ConceptParentsSerializer def get_queryset(self): diff --git a/core/integration_tests/tests_cross_repo_access.py b/core/integration_tests/tests_cross_repo_access.py index d25fce697..4aaf7bda3 100644 --- a/core/integration_tests/tests_cross_repo_access.py +++ b/core/integration_tests/tests_cross_repo_access.py @@ -215,6 +215,12 @@ def test_parents_and_children_lists_hide_private_concepts(self): self.assertEqual(response.status_code, 200) self.assertEqual(sorted(concept['id'] for concept in response.data), children, user) + # these lists aren't searchable: a search would list the repo's concepts instead + for url, expected in [(child.uri + 'parents/?q=secret', parents), + (self.public_concept.uri + 'children/?q=privchild', children)]: + response = self.client.get(url, **headers) + self.assertEqual(sorted(concept['id'] for concept in response.data), expected, (user, url)) + class MappingTargetAccessTest(CrossRepoAccessBaseTest): def test_outsider_mapping_to_private_concept_is_external(self): From 85fcd75ecbb8532bad494ff6b4da1b9510770c1d Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 01:24:38 -0400 Subject: [PATCH 27/35] OpenConceptLab/ocl_online#224 | Repo search in the requester's own scope excludes other owners' private content A search of a repo the requester owns or is a member of includes private content, but the owner and source filters match names in any case, and owner names that differ only in case belong to other users or orgs. Their private content is now excluded: concepts by exact-case owner name; for mappings, which have no exact-case owner field, private content is left out when such an owner exists. Co-Authored-By: Claude Opus 5.5 --- core/common/views.py | 30 +++++++++++++++--- .../tests_search_resolution_access.py | 31 +++++++++++++++++++ 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/core/common/views.py b/core/common/views.py index d5483f580..111215e7a 100644 --- a/core/common/views.py +++ b/core/common/views.py @@ -29,7 +29,7 @@ CANONICAL_URL_REQUEST_PARAM, CHECKSUMS_PARAM, ACCESS_TYPE_NONE from core.common.exceptions import Http400 from core.common.mixins import PathWalkerMixin -from core.common.search import CustomESSearch +from core.common.search import CustomESSearch, EXACT_OWNER_FIELD from core.common.serializers import RootSerializer from core.common.swagger_parameters import all_resource_query_param from core.common.throttling import ThrottleUtil @@ -721,6 +721,27 @@ def __should_query_latest_version(self): return (not collection or collection.startswith('!')) and (not version or version.startswith('!')) + def get_scope_owner_criteria(self): + """Private content in the requester's own scope (their user, or an org of theirs) is shown, but the owner and + source filters match names in any case, and owner names that differ only in case are other owners: their + private content is excluded (concepts by exact owner name; for other documents, all private content is + excluded when such an owner exists). None when there's nothing to exclude.""" + if (not self.is_source_child_document_model() or self.request.user.is_staff or 'collection' in self.kwargs or + not self.is_user_scope()): + return None + org = self.kwargs.get('org') + owner = org or self.kwargs.get('user') or self.request.user.username + public = Q('term', public_can_view=True) + if self.is_concept_document(): + return public | Q('term', **{EXACT_OWNER_FIELD: owner}) + if org: + from core.orgs.models import Organization + has_twin = Organization.objects.filter(mnemonic__iexact=org).exclude(mnemonic=org).exists() + else: + from core.users.models import UserProfile + has_twin = UserProfile.objects.filter(username__iexact=owner).exclude(username=owner).exists() + return public if has_twin else None + def __apply_common_search_filters(self, ignore_retired_filter=False, force=False): results = None if not force and not self.should_perform_es_search(): @@ -758,9 +779,10 @@ def __apply_common_search_filters(self, ignore_retired_filter=False, force=False if not ignore_retired_filter and self._should_exclude_retired_from_search_results(): results = results.filter('term', retired=False) - include_private = self._should_include_private() - if not include_private: - results = results.filter(self.get_public_criteria()) + visibility_criteria = self.get_scope_owner_criteria() if self._should_include_private() else ( + self.get_public_criteria()) + if visibility_criteria is not None: + results = results.filter(visibility_criteria) faceted_criterion = self.get_faceted_criterion() if faceted_criterion: results = results.filter(faceted_criterion) diff --git a/core/integration_tests/tests_search_resolution_access.py b/core/integration_tests/tests_search_resolution_access.py index 45016338e..9268f9d40 100644 --- a/core/integration_tests/tests_search_resolution_access.py +++ b/core/integration_tests/tests_search_resolution_access.py @@ -8,6 +8,9 @@ from core.concepts.models import Concept from core.concepts.search import ConceptFuzzySearch from core.concepts.tests.factories import ConceptFactory +from core.mappings.documents import MappingDocument +from core.mappings.models import Mapping +from core.mappings.tests.factories import MappingFactory from core.orgs.tests.factories import OrganizationFactory from core.sources.tests.factories import OrganizationSourceFactory from core.url_registry.factories import GlobalURLRegistryFactory @@ -157,6 +160,34 @@ def test_match_head_target_repo_with_bare_url_searches_head(self): self.assertEqual(search_mock.call_args[0][2]['source_version'], 'HEAD') +class RepoSearchOwnerCaseTest(SearchResolutionAccessBaseTest): + def test_repo_search_excludes_private_content_of_owner_whose_name_differs_only_in_case(self): + self.patch_concept_es_mapping_for_ci() + own_org = OrganizationFactory(mnemonic='resolveorg', public_access=ACCESS_TYPE_NONE) + own_org.members.add(self.outsider) + own_source = OrganizationSourceFactory( + organization=own_org, mnemonic='ResolveSource', public_access=ACCESS_TYPE_NONE) + for source in [own_source, self.private_source]: + concept = ConceptFactory(parent=source, mnemonic='case-twin', public_access=ACCESS_TYPE_NONE) + source.concepts.add(concept.get_latest_version()) + mapping = MappingFactory( + parent=source, from_concept=concept, to_concept=concept, public_access=ACCESS_TYPE_NONE) + source.mappings.add(mapping.get_latest_version()) + ConceptDocument().update(Concept.objects.filter(mnemonic='case-twin'), refresh=True) + MappingDocument().update(Mapping.objects.filter(parent__mnemonic='ResolveSource'), refresh=True) + + def search(resource): + response = self.client.get( + f'{own_source.uri}{resource}/?sortAsc=last_update&limit=100', + HTTP_AUTHORIZATION='Token ' + self.outsider.get_token()) + self.assertEqual(response.status_code, 200) + return {result['url'].split('/sources/')[0] for result in response.data} + + self.assertEqual(search('concepts'), {'/orgs/resolveorg'}) + # mappings have no exact-case owner field: with such an owner, only public mappings are searched + self.assertEqual(search('mappings'), set()) + + class ResolveReferenceAccessTest(SearchResolutionAccessBaseTest): def resolve(self, user): response = self.client.post( From 2b727cef2919cf90b72c8434a921c4fa8556bfc2 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 01:24:54 -0400 Subject: [PATCH 28/35] OpenConceptLab/ocl_online#224 | Review cleanups: one collection_versions method, fewer one-caller helpers - CollectionVersionsMixin replaces four copies of get_collection_versions; the unused collection_versions property goes, and the user is required. - GraphQL's global queryset reuses get_viewable_concepts. - get_viewable_parent_uris takes linked_uris as a callable only; its empty guard and CodeSystem $validate-code's earlier empty check were redundant. - Organization.get_visible_to takes the queryset its callers always pass. - The CSV export test's export service no longer stubs the removed lookup. Co-Authored-By: Claude Opus 5.5 --- core/code_systems/views.py | 3 --- core/common/mixins.py | 6 +----- core/common/serializers.py | 6 ++++++ core/concepts/models.py | 7 ++----- core/concepts/serializers.py | 14 +++----------- core/graphql/queries.py | 9 ++------- core/integration_tests/tests_access_control.py | 4 +--- core/mappings/serializers.py | 14 +++----------- core/orgs/models.py | 6 ++---- 9 files changed, 20 insertions(+), 49 deletions(-) diff --git a/core/code_systems/views.py b/core/code_systems/views.py index 1bda2d71b..52b8672ac 100644 --- a/core/code_systems/views.py +++ b/core/code_systems/views.py @@ -165,9 +165,6 @@ def get_queryset(self): url = IdentifierSerializer.convert_fhir_url_to_ocl_uri(url, 'sources') source = Source.objects.filter(uri=url) - if not source: - return queryset.none() - if version: source = source.filter(version=version) else: diff --git a/core/common/mixins.py b/core/common/mixins.py index 991b386dd..b5b105c7c 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -724,11 +724,7 @@ def get_filters_for_criterion(params, prefix=None): def source_versions(self): return self.sources.exclude(version=HEAD).values_list('uri', flat=True) - @property - def collection_versions(self): - return self.get_collection_versions() - - def get_collection_versions(self, user=None): + def get_collection_versions(self, user): """URIs of the released collection versions holding this resource that the user can view.""" expansions = self.filter_by_viewable_collection_version( self.expansion_set.exclude(collection_version__version=HEAD), user) diff --git a/core/common/serializers.py b/core/common/serializers.py index bc00f63e8..984c699b9 100644 --- a/core/common/serializers.py +++ b/core/common/serializers.py @@ -26,6 +26,12 @@ class TaskSerializer(Serializer): # pylint: disable=abstract-method pass +class CollectionVersionsMixin: # pylint: disable=too-few-public-methods + def get_collection_versions(self, obj): + # a $cascade bundle passes its requester as `user` + return obj.get_collection_versions(get(self, 'context.request.user') or get(self, 'context.user')) + + class ReadSerializerMixin: """ Mixin for serializer which does not update or create resources. """ diff --git a/core/concepts/models.py b/core/concepts/models.py index ec40e7dd2..1f47ca05d 100644 --- a/core/concepts/models.py +++ b/core/concepts/models.py @@ -679,11 +679,9 @@ def create_new_version_for( def get_viewable_parent_uris(uris, user, linked_uris=None): """Only parents in repos the user can view can be linked. Parents already linked are kept, and so are not-yet-existing ones pointing into a repo the user can view (linked later by the hierarchy task). - `linked_uris` can be a callable, so it's only evaluated when some parent isn't viewable.""" + `linked_uris` is a callable returning those, called only when some parent isn't viewable.""" from core.common.utils import to_parent_uri from core.sources.models import Source - if not uris: - return [] viewable_uris = set() can_view_source = {} existing_uris = set() @@ -704,8 +702,7 @@ def get_viewable_parent_uris(uris, user, linked_uris=None): if repo_view_by_uri[repo_uri]: viewable_uris.add(uri) if linked_uris and any(uri not in viewable_uris for uri in uris): - linked_uris = set(linked_uris() if callable(linked_uris) else linked_uris) - viewable_uris |= linked_uris + viewable_uris |= set(linked_uris()) return [uri for uri in uris if uri in viewable_uris] def set_parent_concepts_from_uris(self, create_parent_version=True): diff --git a/core/concepts/serializers.py b/core/concepts/serializers.py index b199a069f..fea8cef06 100644 --- a/core/concepts/serializers.py +++ b/core/concepts/serializers.py @@ -9,7 +9,7 @@ INCLUDE_CHILD_CONCEPT_URLS, HEAD, INCLUDE_SUMMARY, INCLUDE_VERBOSE_REFERENCES, VERBOSE_PARAM, \ INCLUDE_RETIRED_PARAM, ACCESS_TYPE_NONE from core.common.fields import EncodedDecodedCharField -from core.common.serializers import AbstractResourceSerializer +from core.common.serializers import AbstractResourceSerializer, CollectionVersionsMixin from core.common.utils import to_parent_uri_from_kwargs, get_truthy_values from core.concepts.models import Concept, ConceptName @@ -332,7 +332,7 @@ class Meta: ) -class ConceptVersionListSerializer(ConceptListSerializer): +class ConceptVersionListSerializer(CollectionVersionsMixin, ConceptListSerializer): previous_version_url = CharField(read_only=True, source='prev_version_uri') source_versions = ListField(read_only=True) collection_versions = SerializerMethodField() @@ -362,10 +362,6 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - def get_collection_versions(self, obj): - # a $cascade bundle passes its requester as `user` - return obj.get_collection_versions(get(self, 'context.request.user') or get(self, 'context.user')) - class ConceptVersionCascadeSerializer(ConceptVersionListSerializer): class Meta: @@ -577,7 +573,7 @@ def get_child_concept_urls(obj): return list(uris) -class ConceptVersionDetailSerializer(ModelSerializer): +class ConceptVersionDetailSerializer(CollectionVersionsMixin, ModelSerializer): type = CharField(source='resource_type') uuid = CharField(source='id') id = EncodedDecodedCharField(source='mnemonic') @@ -693,10 +689,6 @@ def get_parent_concepts(self, obj): get_viewable_concepts(obj.parent_concepts.all(), get(self, 'context.request')), many=True).data return None - def get_collection_versions(self, obj): - # a $cascade bundle passes its requester as `user` - return obj.get_collection_versions(get(self, 'context.request.user') or get(self, 'context.user')) - class ConceptHierarchySerializer(ModelSerializer): uuid = CharField(source='id') diff --git a/core/graphql/queries.py b/core/graphql/queries.py index 14a3ae035..3f1e9a0ea 100644 --- a/core/graphql/queries.py +++ b/core/graphql/queries.py @@ -17,6 +17,7 @@ from core.common.search import get_visible_repo_criteria from core.concepts.documents import ConceptDocument from core.concepts.models import Concept +from core.concepts.serializers import get_viewable_concepts from core.mappings.models import Mapping from core.sources.models import Source @@ -105,12 +106,6 @@ def build_mapping_prefetch(source_version: Source) -> Prefetch: return Prefetch('mappings_from', queryset=mapping_qs, to_attr='graphql_mappings') -def build_global_base_queryset(user): - """Active concepts across all repositories the user can view.""" - queryset = Concept.objects.filter(is_active=True, retired=False) - return queryset if user.is_staff else Concept.apply_user_criteria(queryset, user) - - def build_global_mapping_prefetch(user) -> Prefetch: """Mappings from each concept, in repositories the user can view.""" mapping_qs = Mapping.objects.filter( @@ -514,7 +509,7 @@ async def concepts( # pylint: disable=too-many-arguments,too-many-locals else: # Global search across all repositories the user can view source_version = None - base_qs = build_global_base_queryset(user) + base_qs = get_viewable_concepts(Concept.objects.filter(is_active=True, retired=False), info.context) mapping_prefetch = build_global_mapping_prefetch(user) if concept_ids_param: diff --git a/core/integration_tests/tests_access_control.py b/core/integration_tests/tests_access_control.py index 84d0677ad..47c0f3883 100644 --- a/core/integration_tests/tests_access_control.py +++ b/core/integration_tests/tests_access_control.py @@ -564,9 +564,7 @@ class CsvExportAccessTest(AccessTestMixin, OCLAPITestCase): def setUp(self): super().setUp() self.exports = {} - export_service = Mock( - upload_file=self.upload, exists=lambda key: key in self.exports, - url_for=lambda key: 'https://exports.test/' + key) + export_service = Mock(upload_file=self.upload, url_for=lambda key: 'https://exports.test/' + key) patcher = patch('core.common.utils.get_export_service', return_value=export_service) patcher.start() self.addCleanup(patcher.stop) diff --git a/core/mappings/serializers.py b/core/mappings/serializers.py index 291fa3de2..a1ebdabe9 100644 --- a/core/mappings/serializers.py +++ b/core/mappings/serializers.py @@ -7,7 +7,7 @@ MAPPING_LOOKUP_TO_CONCEPT, MAPPING_LOOKUP_FROM_SOURCE, MAPPING_LOOKUP_TO_SOURCE, INCLUDE_EXTRAS_PARAM, \ INCLUDE_SOURCE_VERSIONS, INCLUDE_COLLECTION_VERSIONS, INCLUDE_VERBOSE_REFERENCES from core.common.fields import EncodedDecodedCharField -from core.common.serializers import AbstractResourceSerializer +from core.common.serializers import AbstractResourceSerializer, CollectionVersionsMixin from core.common.utils import get_truthy_values from core.concepts.serializers import ConceptDetailSerializer from core.mappings.models import Mapping @@ -138,7 +138,7 @@ class Meta: ) -class MappingVersionListSerializer(MappingListSerializer): +class MappingVersionListSerializer(CollectionVersionsMixin, MappingListSerializer): previous_version_url = CharField(read_only=True, source='prev_version_uri') source_versions = ListField(read_only=True) collection_versions = SerializerMethodField() @@ -162,10 +162,6 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - def get_collection_versions(self, obj): - # a $cascade bundle passes its requester as `user` - return obj.get_collection_versions(get(self, 'context.request.user') or get(self, 'context.user')) - class MappingChecksumSerializer(AbstractResourceSerializer): id = CharField(source='mnemonic', read_only=True) @@ -319,7 +315,7 @@ def get_checksums(obj): return obj.get_checksums() -class MappingVersionDetailSerializer(MappingDetailSerializer): +class MappingVersionDetailSerializer(CollectionVersionsMixin, MappingDetailSerializer): previous_version_url = CharField(read_only=True, source='prev_version_uri') source_versions = ListField(read_only=True) collection_versions = SerializerMethodField() @@ -329,7 +325,3 @@ class Meta: fields = MappingDetailSerializer.Meta.fields + ( 'previous_version_url', 'source_versions', 'collection_versions', ) - - def get_collection_versions(self, obj): - # a $cascade bundle passes its requester as `user` - return obj.get_collection_versions(get(self, 'context.request.user') or get(self, 'context.user')) diff --git a/core/orgs/models.py b/core/orgs/models.py index 92a59e02a..33caba85b 100644 --- a/core/orgs/models.py +++ b/core/orgs/models.py @@ -81,10 +81,8 @@ def get_public(cls): return cls.objects.filter(public_access=ACCESS_TYPE_VIEW) @classmethod - def get_visible_to(cls, user, queryset=None): - """Limits `queryset` (default: all orgs) to the orgs `user` can view: public orgs and the user's own. - Staff can view all.""" - queryset = cls.objects.all() if queryset is None else queryset + def get_visible_to(cls, user, queryset): + """Limits `queryset` to the orgs `user` can view: public orgs and the user's own. Staff can view all.""" if get(user, 'is_superuser') or get(user, 'is_staff'): return queryset visible = cls.get_public() From 1de3d3e250bda706b1e93334e413238bce178666 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 01:56:24 -0400 Subject: [PATCH 29/35] OpenConceptLab/ocl_online#224 | $cascade and $clone check view access on the repo they run over The root concept is resolved from the merged URL kwargs and query params, and its own repo is checked, but the repo the cascade or clone runs over comes from the URL. That repo now needs view access too (404 otherwise). Co-Authored-By: Claude Opus 5.5 --- core/concepts/views.py | 11 ++++++++-- .../tests_cross_repo_access.py | 20 +++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/core/concepts/views.py b/core/concepts/views.py index 667b30592..f70a52b69 100644 --- a/core/concepts/views.py +++ b/core/concepts/views.py @@ -542,9 +542,16 @@ def get_object(self, queryset=None): omit_if_exists_in_param, equivalency_map_types_param ] ) + def set_cascade_repo(self): + """The repo the cascade runs over, from the URL. Query params can pick the root concept from another repo, + so this one's view access is checked too.""" + self.set_parent_resource(False) + if self.parent_resource and not self.parent_resource.has_view_access(self.request.user): + raise Http404() + def get(self, request, **kwargs): # pylint: disable=unused-argument instance = self.get_object() - self.set_parent_resource(False) + self.set_cascade_repo() bundle = Bundle( root=instance, params=self.request.query_params, verbose=self.is_verbose(), repo_version=self.parent_resource, requested_url=self.request.get_full_path(), user=self.request.user @@ -565,7 +572,7 @@ def post(self, request, **kwargs): # pylint: disable=unused-argument } """ clone_to_source = self.get_clone_to_source() - self.set_parent_resource(False) + self.set_cascade_repo() budget = get_clone_budget(request.user) parameters = dict(request.data.get('parameters') or {}) parameters.pop('resource_budget', None) diff --git a/core/integration_tests/tests_cross_repo_access.py b/core/integration_tests/tests_cross_repo_access.py index 4aaf7bda3..d2b6c4742 100644 --- a/core/integration_tests/tests_cross_repo_access.py +++ b/core/integration_tests/tests_cross_repo_access.py @@ -397,3 +397,23 @@ def test_reference_ignores_omit_repo_its_creator_cannot_view(self): created_by=user) self.assertEqual('omit_if_exists_in' in reference.get_concept_cascade_params(), kept, user) + + +class CascadeRepoAccessTest(CrossRepoAccessBaseTest): + def test_cascade_and_clone_over_a_repo_the_requester_cannot_view_are_not_found(self): + MappingFactory(parent=self.private_source, from_concept=self.private_concept, to_concept=self.public_concept) + # the root concept comes from the query params, the repo to cascade over from the URL + url = (f'{self.private_concept.uri}$cascade/?org={self.public_source.organization.mnemonic}' + f'&source={self.public_source.mnemonic}&concept={self.public_concept.mnemonic}&reverse=true') + + self.assertEqual(self.client.get(url).status_code, 404) + self.assertEqual(self.client.get(url, HTTP_AUTHORIZATION='Token ' + self.outsider_token).status_code, 404) + response = self.client.get(url, HTTP_AUTHORIZATION='Token ' + self.member.get_token()) + self.assertEqual(response.status_code, 200) + self.assertIn(self.private_concept.uri, str(response.data)) + + response = self.client.post( + url.replace('$cascade/', '$clone/'), + {'source_uri': self.outsider_source.uri, 'parameters': {'reverse': True}}, + HTTP_AUTHORIZATION='Token ' + self.outsider_token, format='json') + self.assertEqual(response.status_code, 404) From c3c77cbfede6157071ef97a3284486e98e108b9f Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 01:56:24 -0400 Subject: [PATCH 30/35] OpenConceptLab/ocl_online#224 | Repo list search applies the case rule too; new org names are unique regardless of case Source, collection and repo list searches in the requester's own scope match owner names in any case, like concept and mapping searches: when another owner of the same type has the same name in a different case, only public repos are searched. New orgs can't take a name that differs from an existing org's only in case, so no new such owners can be created. Co-Authored-By: Claude Opus 5.5 --- core/common/views.py | 9 ++++---- .../tests_search_resolution_access.py | 23 +++++++++++++++++++ core/orgs/serializers.py | 2 +- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/core/common/views.py b/core/common/views.py index 111215e7a..3018a7405 100644 --- a/core/common/views.py +++ b/core/common/views.py @@ -724,10 +724,11 @@ def __should_query_latest_version(self): def get_scope_owner_criteria(self): """Private content in the requester's own scope (their user, or an org of theirs) is shown, but the owner and source filters match names in any case, and owner names that differ only in case are other owners: their - private content is excluded (concepts by exact owner name; for other documents, all private content is - excluded when such an owner exists). None when there's nothing to exclude.""" - if (not self.is_source_child_document_model() or self.request.user.is_staff or 'collection' in self.kwargs or - not self.is_user_scope()): + private content is excluded (concepts by exact owner name; for other documents, such as mappings and repos, + all private content is excluded when such an owner exists). None when there's nothing to exclude.""" + if (not (self.is_source_child_document_model() or self.is_concept_container_document_model() or + self.is_repo_document_model()) or + self.request.user.is_staff or 'collection' in self.kwargs or not self.is_user_scope()): return None org = self.kwargs.get('org') owner = org or self.kwargs.get('user') or self.request.user.username diff --git a/core/integration_tests/tests_search_resolution_access.py b/core/integration_tests/tests_search_resolution_access.py index 9268f9d40..618c009e1 100644 --- a/core/integration_tests/tests_search_resolution_access.py +++ b/core/integration_tests/tests_search_resolution_access.py @@ -12,6 +12,8 @@ from core.mappings.models import Mapping from core.mappings.tests.factories import MappingFactory from core.orgs.tests.factories import OrganizationFactory +from core.sources.documents import SourceDocument +from core.sources.models import Source from core.sources.tests.factories import OrganizationSourceFactory from core.url_registry.factories import GlobalURLRegistryFactory from core.users.models import UserProfile @@ -188,6 +190,27 @@ def search(resource): self.assertEqual(search('mappings'), set()) +class RepoListSearchOwnerCaseTest(SearchResolutionAccessBaseTest): + def test_repo_list_search_excludes_private_repos_of_owner_whose_name_differs_only_in_case(self): + own_org = OrganizationFactory(mnemonic='resolveorg', public_access=ACCESS_TYPE_NONE) + own_org.members.add(self.outsider) + own_source = OrganizationSourceFactory(organization=own_org, mnemonic='ResolveSource') + SourceDocument().update(Source.objects.filter(mnemonic='ResolveSource'), refresh=True) + + response = self.client.get( + '/orgs/resolveorg/sources/?q=ResolveSource', HTTP_AUTHORIZATION='Token ' + self.outsider.get_token()) + + self.assertEqual(response.status_code, 200) + self.assertEqual([source['url'] for source in response.data], [own_source.uri]) + + def test_org_names_are_unique_regardless_of_case(self): + response = self.client.post( + '/orgs/', {'id': 'RESOLVEORG', 'name': 'Twin'}, HTTP_AUTHORIZATION='Token ' + self.outsider.get_token(), + format='json') + + self.assertEqual(response.status_code, 400) + + class ResolveReferenceAccessTest(SearchResolutionAccessBaseTest): def resolve(self, user): response = self.client.post( diff --git a/core/orgs/serializers.py b/core/orgs/serializers.py index c6002ecc3..b12f8db37 100644 --- a/core/orgs/serializers.py +++ b/core/orgs/serializers.py @@ -48,7 +48,7 @@ class Meta: def prepare_object(self, validated_data): user = self.context['request'].user mnemonic = validated_data.get('mnemonic', None) - if Organization.objects.filter(mnemonic=mnemonic).exists(): + if Organization.objects.filter(mnemonic__iexact=mnemonic).exists(): # unique regardless of case self._errors['mnemonic'] = f'Organization with mnemonic {mnemonic} already exists.' return Organization() organization = Organization(name=validated_data.get('name'), mnemonic=mnemonic) From 66090dc80d507a16a00f72b8e8ce694e16e46cca Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 01:56:24 -0400 Subject: [PATCH 31/35] OpenConceptLab/ocl_online#224 | Event descriptions follow the same live-object check; no CSV export of events An event's description names its referenced object, so it's only the event type when the requester can't view that object now. Event lists can't be exported as CSV, which would skip the serializer. Co-Authored-By: Claude Opus 5.5 --- core/events/serializers.py | 5 +++++ core/events/views.py | 1 + core/integration_tests/tests_events.py | 16 +++++++++++----- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/core/events/serializers.py b/core/events/serializers.py index c93cce560..eb46111b6 100644 --- a/core/events/serializers.py +++ b/core/events/serializers.py @@ -9,6 +9,7 @@ class EventSerializer(serializers.ModelSerializer): actor = UserListSerializer(read_only=True) referenced_object = serializers.SerializerMethodField() object = serializers.SerializerMethodField() + description = serializers.SerializerMethodField() class Meta: model = Event @@ -31,6 +32,10 @@ def get_referenced_object(self, obj): data = obj._referenced_object return data + def get_description(self, obj): + referenced_object = obj.referenced_object + return obj.event_type if referenced_object and not self.can_view(referenced_object) else obj.description + def get_object(self, obj): resource_object = obj.object data = None diff --git a/core/events/views.py b/core/events/views.py index 0ef104dda..94ae20801 100644 --- a/core/events/views.py +++ b/core/events/views.py @@ -18,6 +18,7 @@ class EventsView(BaseAPIView, ListWithHeadersMixin): is_searchable = False + allow_csv_export = False # an export would skip the serializer's live-object checks permission_classes = (AllowAny,) default_qs_sort_attr = '-created_at' serializer_class = EventSerializer diff --git a/core/integration_tests/tests_events.py b/core/integration_tests/tests_events.py index 8da704fc2..d10e0a875 100644 --- a/core/integration_tests/tests_events.py +++ b/core/integration_tests/tests_events.py @@ -288,16 +288,22 @@ def test_get_events_show_objects_only_to_requesters_who_can_view_them_now(self): source.public_access = 'None' source.save() - def get_referenced_objects(user=None): + def get_events(user=None, params=''): headers = {'HTTP_AUTHORIZATION': f'Token {user.get_token()}'} if user else {} - response = self.client.get('/users/turned-follower/events/', **headers) + response = self.client.get(f'/users/turned-follower/events/{params}', **headers) self.assertEqual(response.status_code, 200) - return [event['referenced_object'] for event in response.data] + return 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 for user in [None, follower]: - self.assertEqual(get_referenced_objects(user), [None], user) - self.assertEqual([obj['url'] for obj in get_referenced_objects(member)], [source.url]) + 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') def test_get_orgs_events(self): # pylint:disable=too-many-statements,too-many-locals alfred = UserProfileFactory(username='alfred', first_name='Alfred', last_name='Pennyworth') From 06da5d828b9e8d774a6b3d9b1840e87dd2a79412 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 01:56:24 -0400 Subject: [PATCH 32/35] OpenConceptLab/ocl_online#224 | A collection's mappings for a concept match exact URL prefixes The concept and collection URLs were matched as case-insensitive substrings, so a collection whose URL differed only in case (another owner's) contributed its mappings. They now match by exact prefix, which can also use the URL indexes. Co-Authored-By: Claude Opus 5.5 --- core/concepts/models.py | 8 ++++---- core/concepts/tests/tests.py | 19 +++++++++++++++++-- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/core/concepts/models.py b/core/concepts/models.py index 1f47ca05d..5012d7e75 100644 --- a/core/concepts/models.py +++ b/core/concepts/models.py @@ -1283,16 +1283,16 @@ def validate_locales_limit(names, descriptions): def get_unidirectional_mappings_for_collection(self, collection_url, collection_version=HEAD): from core.mappings.models import Mapping return Mapping.objects.filter( - from_concept__uri__icontains=drop_version(self.uri), - expansion_set__collection_version__uri__icontains=collection_url, + from_concept__uri__startswith=drop_version(self.uri), + expansion_set__collection_version__uri__startswith=collection_url, expansion_set__collection_version__version=collection_version ) def get_indirect_mappings_for_collection(self, collection_url, collection_version=HEAD): from core.mappings.models import Mapping return Mapping.objects.filter( - to_concept__uri__icontains=drop_version(self.uri), - expansion_set__collection_version__uri__icontains=collection_url, + to_concept__uri__startswith=drop_version(self.uri), + expansion_set__collection_version__uri__startswith=collection_url, expansion_set__collection_version__version=collection_version ) diff --git a/core/concepts/tests/tests.py b/core/concepts/tests/tests.py index 513191090..aea0c8a3b 100644 --- a/core/concepts/tests/tests.py +++ b/core/concepts/tests/tests.py @@ -32,6 +32,7 @@ from core.concepts.validators import ValidatorSpecifier from core.mappings.models import Mapping from core.mappings.tests.factories import MappingFactory +from core.orgs.tests.factories import OrganizationFactory from core.sources.tests.factories import OrganizationSourceFactory from core.users.models import UserProfile from core.users.tests.factories import UserProfileFactory @@ -397,7 +398,6 @@ def test_match_concepts_access_denied_403(self): def test_match_concepts_consumption_attributed_from_event_metadata(self, _filter_queryset_mock): from core.capabilities.models import UsageEvent from core.map_projects.tests.factories import MapProjectFactory - from core.orgs.tests.factories import OrganizationFactory user = UserProfileFactory() user.groups.add(Group.objects.get(name=PREVIEW_GROUP_NAME)) org = OrganizationFactory() @@ -422,7 +422,6 @@ def test_match_concepts_ignores_map_project_id_the_user_does_not_own(self, _filt # usage still charges the right user's quota, it just stays unattributed. from core.capabilities.models import UsageEvent from core.map_projects.tests.factories import MapProjectFactory - from core.orgs.tests.factories import OrganizationFactory user = UserProfileFactory() user.groups.add(Group.objects.get(name=PREVIEW_GROUP_NAME)) other_user = UserProfileFactory() @@ -2760,9 +2759,17 @@ def test_get_unidirectional_mappings_for_collection(self): expansion = ExpansionFactory(collection_version=collection) expansion.mappings.add(mapping) + twin_mapping = MappingFactory(from_concept=concept1, to_concept=concept2, parent=source) + twin = OrganizationCollectionFactory( + organization=OrganizationFactory(mnemonic=collection.organization.mnemonic.upper()), + mnemonic=collection.mnemonic.upper()) + ExpansionFactory(collection_version=twin).mappings.add(twin_mapping) + results = concept1.get_unidirectional_mappings_for_collection(collection.uri) self.assertIn(mapping.id, results.values_list('id', flat=True)) + # a collection whose URL differs only in case is another collection + self.assertNotIn(twin_mapping.id, results.values_list('id', flat=True)) def test_get_indirect_mappings_for_collection(self): source = OrganizationSourceFactory() @@ -2773,9 +2780,17 @@ def test_get_indirect_mappings_for_collection(self): expansion = ExpansionFactory(collection_version=collection) expansion.mappings.add(mapping) + twin_mapping = MappingFactory(from_concept=concept1, to_concept=concept2, parent=source) + twin = OrganizationCollectionFactory( + organization=OrganizationFactory(mnemonic=collection.organization.mnemonic.upper()), + mnemonic=collection.mnemonic.upper()) + ExpansionFactory(collection_version=twin).mappings.add(twin_mapping) + results = concept2.get_indirect_mappings_for_collection(collection.uri) self.assertIn(mapping.id, results.values_list('id', flat=True)) + # a collection whose URL differs only in case is another collection + self.assertNotIn(twin_mapping.id, results.values_list('id', flat=True)) def test_get_hierarchy_concept_urls_versioned(self): source = OrganizationSourceFactory() From bc5fdebf0f8a7d0c225341e1baec40c98fd75769 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 02:07:58 -0400 Subject: [PATCH 33/35] OpenConceptLab/ocl_online#224 | The case rule covers /user/orgs/ repo lists, which search all the requester's orgs Co-Authored-By: Claude Opus 5.5 --- core/common/views.py | 17 +++++++++++++---- .../tests_search_resolution_access.py | 8 ++++---- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/core/common/views.py b/core/common/views.py index 3018a7405..27dd43e0c 100644 --- a/core/common/views.py +++ b/core/common/views.py @@ -721,6 +721,11 @@ def __should_query_latest_version(self): return (not collection or collection.startswith('!')) and (not version or version.startswith('!')) + def is_members_view(self): + """/user/orgs//: lists across all the requester's orgs.""" + return self.get_view_name() in ['Organization Collection List', 'Organization Source List', + 'Organization Repo List', 'Organization Url Registry List'] + def get_scope_owner_criteria(self): """Private content in the requester's own scope (their user, or an org of theirs) is shown, but the owner and source filters match names in any case, and owner names that differ only in case are other owners: their @@ -733,11 +738,16 @@ def get_scope_owner_criteria(self): org = self.kwargs.get('org') owner = org or self.kwargs.get('user') or self.request.user.username public = Q('term', public_can_view=True) + orgs = list(self.request.user.organizations.values_list('mnemonic', flat=True)) if ( + self.is_members_view()) else ([org] if org else None) if self.is_concept_document(): return public | Q('term', **{EXACT_OWNER_FIELD: owner}) - if org: + if orgs is not None: from core.orgs.models import Organization - has_twin = Organization.objects.filter(mnemonic__iexact=org).exclude(mnemonic=org).exists() + twin_criteria = models.Q() + for mnemonic in orgs: + twin_criteria |= models.Q(mnemonic__iexact=mnemonic) + has_twin = bool(orgs) and Organization.objects.filter(twin_criteria).exclude(mnemonic__in=orgs).exists() else: from core.users.models import UserProfile has_twin = UserProfile.objects.filter(username__iexact=owner).exclude(username=owner).exists() @@ -901,8 +911,7 @@ def __get_search_results(self, ignore_retired_filter=False, sort=True, highlight is_authenticated = user.is_authenticated username = user.username - is_members_view = self.get_view_name() in ['Organization Collection List', 'Organization Source List', - 'Organization Repo List', 'Organization Url Registry List'] + is_members_view = self.is_members_view() if self.is_owner_document_model(): kwargs_filters = self.kwargs.copy() if self.user_is_self and is_authenticated: diff --git a/core/integration_tests/tests_search_resolution_access.py b/core/integration_tests/tests_search_resolution_access.py index 618c009e1..5335a7589 100644 --- a/core/integration_tests/tests_search_resolution_access.py +++ b/core/integration_tests/tests_search_resolution_access.py @@ -197,11 +197,11 @@ def test_repo_list_search_excludes_private_repos_of_owner_whose_name_differs_onl own_source = OrganizationSourceFactory(organization=own_org, mnemonic='ResolveSource') SourceDocument().update(Source.objects.filter(mnemonic='ResolveSource'), refresh=True) - response = self.client.get( - '/orgs/resolveorg/sources/?q=ResolveSource', HTTP_AUTHORIZATION='Token ' + self.outsider.get_token()) + for url in ['/orgs/resolveorg/sources/?q=ResolveSource', '/user/orgs/sources/?q=ResolveSource']: + response = self.client.get(url, HTTP_AUTHORIZATION='Token ' + self.outsider.get_token()) - self.assertEqual(response.status_code, 200) - self.assertEqual([source['url'] for source in response.data], [own_source.uri]) + self.assertEqual(response.status_code, 200) + self.assertEqual([source['url'] for source in response.data], [own_source.uri], url) def test_org_names_are_unique_regardless_of_case(self): response = self.client.post( From cf7d9a0a3d4fbfdbf903732c0edfbd6a686fc5ef Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 02:07:58 -0400 Subject: [PATCH 34/35] OpenConceptLab/ocl_online#224 | A collection's concept mappings through /user/collections/ resolve the collection's owner The /user/ routes carry no owner in the URL kwargs, so the collection URL the mappings are matched against now names the requester, as the exact prefix match needs. Co-Authored-By: Claude Opus 5.5 --- core/concepts/serializers.py | 13 +++++++++-- core/concepts/views.py | 6 ++--- .../tests_cross_repo_access.py | 22 ++++++++++++++++++- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/core/concepts/serializers.py b/core/concepts/serializers.py index fea8cef06..4c2c8264a 100644 --- a/core/concepts/serializers.py +++ b/core/concepts/serializers.py @@ -17,6 +17,14 @@ TRUTHY = get_truthy_values() +def get_collection_uri_from_view(view, request): + """The URI of the collection in the view's URL, with /user/ resolved to the requester.""" + kwargs = get(view, 'kwargs') or {} + if get(view, 'user_is_self'): + kwargs = {**kwargs, 'user': get(request, 'user.username')} + return to_parent_uri_from_kwargs(kwargs) + + def get_viewable_concepts(queryset, request): """Concepts from repos the requester can view.""" user = get(request, 'user') @@ -236,7 +244,7 @@ def get_mappings(self, obj): serializer_class = MappingMinimalSerializer if is_mapping_brief else MappingDetailSerializer is_collection = 'collection' in self.view_kwargs collection_version = self.view_kwargs.get('version', HEAD) if is_collection else None - parent_uri = to_parent_uri_from_kwargs(self.view_kwargs) if is_collection else None + parent_uri = get_collection_uri_from_view(get(context, 'view'), request) if is_collection else None if self.include_indirect_mappings: mappings = obj.get_bidirectional_mappings_for_collection( parent_uri, collection_version @@ -664,7 +672,8 @@ def get_mappings(self, obj): context = get(self, 'context') is_collection = 'collection' in self.view_kwargs collection_version = self.view_kwargs.get('version', HEAD) if is_collection else None - parent_uri = to_parent_uri_from_kwargs(self.view_kwargs) if is_collection else None + parent_uri = get_collection_uri_from_view( + get(context, 'view'), get(context, 'request')) if is_collection else None if self.include_indirect_mappings: mappings = obj.get_bidirectional_mappings_for_collection( parent_uri, collection_version diff --git a/core/concepts/views.py b/core/concepts/views.py index f70a52b69..41fec4cc6 100644 --- a/core/concepts/views.py +++ b/core/concepts/views.py @@ -45,7 +45,7 @@ match_brief_param, match_encoder_model_param, match_reranker_param, match_offset_param) from core.common.tasks import delete_concept, make_hierarchy from core.common.throttling import ThrottleUtil -from core.common.utils import (to_parent_uri_from_kwargs, generate_temp_version, get_truthy_values, to_int, +from core.common.utils import (generate_temp_version, get_truthy_values, to_int, drop_version, get_falsy_values, parse_id, get_event_metadata) from core.common.views import SourceChildCommonBaseView, SourceChildExtrasView, \ SourceChildExtraRetrieveUpdateDestroyView, BaseAPIView @@ -62,7 +62,7 @@ ConceptVersionDetailSerializer, ConceptVersionListSerializer, ConceptSummarySerializer, ConceptMinimalSerializer, ConceptChildrenSerializer, ConceptParentsSerializer, ConceptLookupListSerializer, ConceptChecksumSerializer, - get_viewable_concepts) + get_viewable_concepts, get_collection_uri_from_view) from core.mappings.serializers import MappingListSerializer from core.sources.clone_limits import clone_limit_error_detail, clone_lock, get_clone_budget from core.sources.models import CloneError, CloneLimitExceeded @@ -701,7 +701,7 @@ def get_queryset(self): include_indirect_mappings = self.request.query_params.get(INCLUDE_INVERSE_MAPPINGS_PARAM, 'false') in TRUTHY is_collection = 'collection' in self.kwargs collection_version = self.kwargs.get('version', HEAD) if is_collection else None - parent_uri = to_parent_uri_from_kwargs(self.kwargs) if is_collection else None + parent_uri = get_collection_uri_from_view(self, self.request) if is_collection else None if include_indirect_mappings: mappings_queryset = concept.get_bidirectional_mappings_for_collection( parent_uri, collection_version diff --git a/core/integration_tests/tests_cross_repo_access.py b/core/integration_tests/tests_cross_repo_access.py index d2b6c4742..cb6de1ca2 100644 --- a/core/integration_tests/tests_cross_repo_access.py +++ b/core/integration_tests/tests_cross_repo_access.py @@ -4,7 +4,7 @@ from core.bundles.models import Bundle from core.collections.models import CollectionReference -from core.collections.tests.factories import OrganizationCollectionFactory, ExpansionFactory +from core.collections.tests.factories import OrganizationCollectionFactory, ExpansionFactory, UserCollectionFactory from core.common.constants import ACCESS_TYPE_NONE from core.common.tests import OCLAPITestCase from core.concepts.models import Concept @@ -417,3 +417,23 @@ def test_cascade_and_clone_over_a_repo_the_requester_cannot_view_are_not_found(s {'source_uri': self.outsider_source.uri, 'parameters': {'reverse': True}}, HTTP_AUTHORIZATION='Token ' + self.outsider_token, format='json') self.assertEqual(response.status_code, 404) + + +class CollectionConceptMappingsTest(CrossRepoAccessBaseTest): + def test_concept_mappings_in_a_collection_through_the_current_user_route(self): + mapping = MappingFactory( + parent=self.public_source, from_concept=self.public_concept, to_concept=self.public_concept) + collection = UserCollectionFactory(user=self.outsider, mnemonic='MyColl') + expansion = ExpansionFactory(collection_version=collection) + collection.expansion_uri = expansion.uri + collection.save() + expansion.concepts.add(self.public_concept.get_latest_version()) + expansion.mappings.add(mapping.get_latest_version()) + + for url in [f'/users/{self.outsider.username}/collections/MyColl/', '/user/collections/MyColl/']: + for param in ['includeMappings', 'includeInverseMappings']: + response = self.client.get( + f'{url}concepts/public/?{param}=true', HTTP_AUTHORIZATION='Token ' + self.outsider_token) + + self.assertEqual(response.status_code, 200, (url, param)) + self.assertEqual({data['url'] for data in response.data['mappings']}, {mapping.url}, (url, param)) From fb7f85494233c4c958a35e72439f1c2526333319 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 14 Oct 2026 02:07:58 -0400 Subject: [PATCH 35/35] OpenConceptLab/ocl_online#224 | Bulk import doesn't create an org whose name differs from an existing one only in case Co-Authored-By: Claude Opus 5.5 --- core/importers/models.py | 2 ++ core/importers/tests.py | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/core/importers/models.py b/core/importers/models.py index c06b2af23..1c41e76bd 100644 --- a/core/importers/models.py +++ b/core/importers/models.py @@ -228,6 +228,8 @@ def parse(self): def process(self): if not self.exists(): + if Organization.objects.filter(mnemonic__iexact=self.data['mnemonic']).exists(): + return FAILED # org names are unique regardless of case org = Organization.objects.create(**self.data) org.members.add(org.created_by) if org: diff --git a/core/importers/tests.py b/core/importers/tests.py index be4080cc0..a169d34df 100644 --- a/core/importers/tests.py +++ b/core/importers/tests.py @@ -2087,6 +2087,14 @@ def test_organization_importer_delete_permission_denied(self): self.assertEqual(importer.delete(), PERMISSION_DENIED) self.assertTrue(Organization.objects.filter(mnemonic=org.mnemonic).exists()) + def test_organization_importer_does_not_create_org_whose_name_differs_only_in_case(self): + OrganizationFactory(mnemonic='TwinOrg') + importer = OrganizationImporter({'id': 'twinorg', 'name': 'Twin'}, UserProfileFactory()) + importer.parse() + + self.assertEqual(importer.process(), FAILED) + self.assertFalse(Organization.objects.filter(mnemonic='twinorg').exists()) + def test_organization_importer_delete_not_found(self): importer = OrganizationImporter({'id': 'DoesNotExist'}, UserProfileFactory()) self.assertEqual(importer.delete(), NOT_FOUND)