diff --git a/docs/architecture.md b/docs/architecture.md index f8c8ebc7..a7ab2886 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -11,3 +11,51 @@ flowchart TD User -- HTTP --> Dashboard --> Opensearch Back -- REST API --> Embedding Endpoint ``` + +### Per-service index topology + +Each registered service gets its own OpenSearch index. The index name is derived from the +`OPENSEARCH_INDEX_PREFIX` setting and the service's name: + +``` +{OPENSEARCH_INDEX_PREFIX}-{service.name} +``` + +``` +Services (bearer token auth) OpenSearch ++------------------+ +------------------+ +| docs service | -- /index/ --> | find-docs | +| | -- /delete/ --> | | ++------------------+ +------------------+ +| drive service | -- /index/ --> | find-drive | +| | -- /delete/ --> | | ++------------------+ +------------------+ +| wiki service | -- /index/ --> | find-wiki | +| | -- /delete/ --> | | ++------------------+ +------------------+ + | + | fan-out across + | all active indices + | +Users (OIDC token auth) v ++------------------+ +------------------+ +| OIDC user | -- /search/ --> | find-docs | +| | | find-drive | +| | | find-wiki | ++------------------+ +------------------+ +``` + +**Services cannot search. Only users (OIDC tokens) can search. Services can delete from their +own index.** + +- `/index/` accepts service bearer tokens (`ServiceTokenAuthentication`). Each service writes + exclusively to its own index. +- `/delete/` accepts service bearer tokens (`ServiceTokenAuthentication`). Each service deletes + exclusively from its own index. +- `/search/` accepts OIDC user tokens (`ResourceServerAuthentication`). Queries fan out across all + indices belonging to `is_active=True` services. + +Setting `Service.is_active=False` removes that service's index from the search fan-out. Documents +in that index become invisible to users until the service is re-activated. + +See [services-cannot-search.md](./services-cannot-search.md) for the full contract. diff --git a/docs/services-cannot-search.md b/docs/services-cannot-search.md new file mode 100644 index 00000000..e1b0b594 --- /dev/null +++ b/docs/services-cannot-search.md @@ -0,0 +1,56 @@ +# Contract: Services Cannot Search + +Services registered in Find can index and delete documents, but they cannot search. Only users +authenticated via OIDC tokens can call `/search/`. + +## Authentication model + +Find exposes three endpoints, each with a distinct authentication requirement: + +| Endpoint | Auth class | Who can call it | +|-------------|-------------------------------|--------------------------| +| `/index/` | `ServiceTokenAuthentication` | Services (bearer token) | +| `/search/` | `ResourceServerAuthentication` + `ResourceServerMixin` | OIDC users only | +| `/delete/` | `ServiceTokenAuthentication` | Services (bearer token) | + +`ServiceTokenAuthentication` validates the `Authorization: Bearer ` header against the +`Service.token` field in the database. It is wired to `IndexDocumentView` and +`DeleteDocumentsView`. + +`ResourceServerAuthentication` (from `django-lasuite`) validates OIDC access tokens issued by +the configured identity provider. It is wired to `SearchDocumentView` via `ResourceServerMixin`. + +## What happens when a service tries to search + +A service bearer token posted to `/search/` is rejected with a 4xx status (typically +**HTTP 400 Bad Request** from `django-lasuite`'s `SuspiciousOperation` when introspection +fails, or **HTTP 401/403** if the token never reaches introspection). + +`ResourceServerAuthentication` does not recognise service bearer tokens. The request is rejected +before any OpenSearch query runs. + +## Why this contract exists + +Services write to and delete from their own isolated index. They have no business reading across +other services' indices. Cross-service fan-out is a user-facing feature: when a user searches, +Find queries all active service indices and merges the results, filtered by the user's identity +(`sub` claim). + +Allowing services to search would break the access-control model: a service could read documents +belonging to users it has no relationship with. + +## Test coverage + +The following test scenarios lock this contract in: + +- `test_api_documents_index.py` verifies that `/index/` rejects OIDC-style bearer tokens + (HTTP 403 `Invalid token.`). +- `test_api_documents_search.py` verifies that `/search/` rejects service bearer tokens and + anonymous requests (HTTP 4xx). +- `test_api_documents_delete.py` verifies that `/delete/` accepts service bearer tokens and rejects + OIDC user tokens. +- `test_integration_per_service_indices.py` verifies the full round-trip: services index and delete + documents from their own indices, users search and find their accessible documents. + +If you add a new endpoint that should be user-only, wire `ResourceServerAuthentication` and +`ResourceServerMixin` to it. Do not add `ServiceTokenAuthentication` to search paths. diff --git a/docs/setup-indexer.md b/docs/setup-indexer.md index 66da7e46..20613ff0 100644 --- a/docs/setup-indexer.md +++ b/docs/setup-indexer.md @@ -12,7 +12,7 @@ Add the following variables to your Django settings to configure Find and enable full-text search. ```python -# Login for opensearch +# Login for opensearch OPENSEARCH_USER=opensearch-user OPENSEARCH_PASSWORD=your-opensearch-password @@ -23,10 +23,33 @@ OPENSEARCH_PORT=9200 # Enable SSL for opensearch connection (False in dev mode) OPENSEARCH_USE_SSL=True -# Unified index name for all services -OPENSEARCH_INDEX=find +# Prefix for per-service indices (each service gets its own index: {prefix}-{service.slug}) +OPENSEARCH_INDEX_PREFIX=find ``` +### Per-service indices + +Find creates one OpenSearch index per registered service. The index name follows the pattern: + +``` +{OPENSEARCH_INDEX_PREFIX}-{service.slug} +``` + +For example, with `OPENSEARCH_INDEX_PREFIX=find`: + +- A service with slug `docs` writes to `find-docs` +- A service with slug `drive` writes to `find-drive` + +Indices are created lazily on the first write. You don't need to create them manually. + +When a service has `is_active=False`, its token can no longer index or delete, and its index is +excluded from search fan-out. Documents in that index are effectively hidden from users until the +service is re-activated. + +`Service.slug` is auto-derived from `name` on creation (lowercase, alphanumeric only) and is +immutable thereafter. `name` is a free-form display field and can be edited freely. If you need +a different slug, create a new service and migrate your documents. + ### Language Language specific operations are applied to document titles and contents to improve search results. @@ -64,13 +87,14 @@ Other applications can index their files through the **`/index/`** endpoint with For each application a new **Service** must be created through the admin interface (see http://localhost:9071/admin/core/service/add/) -| Field | Description | -|-----------------------------|----------------------------------------------------| -| Name | Name of the service (used as `service` field) | -| Is active | Toggle service availability | -| Client id | Calling service client_id (e.g `impress` for docs) | -| Allowed services for search | List of sub-services. Will add the results from all these services
to the search results. | -| Token (_read-only_) | Random token for calling service authentication | +| Field | Description | +|---------------------|----------------------------------------------------------------| +| Name | Human-readable display name (editable) | +| Slug (_read-only_) | Stable identifier used as `service` field and index suffix; auto-derived from Name on creation (lowercase alphanumeric only); immutable thereafter | +| Is active | Toggle service availability (inactive services are excluded from search fan-out) | +| Client id | Calling service client_id (e.g `impress` for docs) | +| Token (_read-only_) | Random token for calling service authentication | +| Allowed services for search | _Currently unused._ Legacy many-to-many field retained on the model for possible future use; search now fans out across all active services regardless of this setting | And add the key in the calling application Django settings. diff --git a/env.d/development/common.dist b/env.d/development/common.dist index 882558bc..01545013 100644 --- a/env.d/development/common.dist +++ b/env.d/development/common.dist @@ -15,7 +15,7 @@ FIND_BASE_URL="http://localhost:8072" # Opensearch OPENSEARCH_PASSWORD=find.PASS123 OPENSEARCH_USE_SSL=false -OPENSEARCH_INDEX=find +OPENSEARCH_INDEX_PREFIX=find # OIDC OIDC_OP_JWKS_ENDPOINT=http://nginx:8083/realms/impress/protocol/openid-connect/certs diff --git a/src/backend/core/authentication.py b/src/backend/core/authentication.py index 2d69361f..39d428ac 100644 --- a/src/backend/core/authentication.py +++ b/src/backend/core/authentication.py @@ -31,7 +31,7 @@ def authenticate(self, request): def authenticate_credentials(self, token): """Check that the token is registered and valid.""" try: - service = self.model.objects.only("name").get(token=token, is_active=True) + service = self.model.objects.only("slug").get(token=token, is_active=True) except self.model.DoesNotExist as excpt: raise exceptions.AuthenticationFailed("Invalid token.") from excpt diff --git a/src/backend/core/factories.py b/src/backend/core/factories.py index ddb13636..10470f68 100644 --- a/src/backend/core/factories.py +++ b/src/backend/core/factories.py @@ -50,7 +50,8 @@ class ServiceFactory(factory.django.DjangoModelFactory): A factory for generating service instances for testing and development purposes. """ - name = factory.Sequence(lambda n: f"test-index-{n!s}") + slug = factory.Sequence(lambda n: f"testidx{n!s}") + name = factory.LazyAttribute(lambda o: f"Test Service {o.slug}") created_at = factory.Faker("date_time_this_year", tzinfo=None) is_active = True client_id = "some_client_id" diff --git a/src/backend/core/migrations/0003_service_slug_and_editable_name.py b/src/backend/core/migrations/0003_service_slug_and_editable_name.py new file mode 100644 index 00000000..05026a00 --- /dev/null +++ b/src/backend/core/migrations/0003_service_slug_and_editable_name.py @@ -0,0 +1,73 @@ +import re + +import django.core.validators +from django.db import migrations, models + + +def backfill_slug_from_name(apps, schema_editor): + Service = apps.get_model("core", "Service") # noqa: N806 + seen = set() + for service in Service.objects.all(): + derived = re.sub(r"[^a-zA-Z0-9]", "", service.name or "").lower() + if not derived: + raise RuntimeError( + f"Cannot derive slug for Service id={service.pk!r} " + f"name={service.name!r}: name contains no alphanumeric characters." + ) + if derived in seen: + raise RuntimeError( + f"Slug collision while backfilling: name={service.name!r} -> " + f"slug={derived!r} already used by another service." + ) + seen.add(derived) + service.slug = derived + service.save(update_fields=["slug"]) + + +class Migration(migrations.Migration): + dependencies = [ + ("core", "0002_service_client_id_service_services"), + ] + + operations = [ + migrations.AddField( + model_name="service", + name="slug", + field=models.CharField(max_length=20, null=True), + ), + migrations.RunPython( + backfill_slug_from_name, + reverse_code=migrations.RunPython.noop, + ), + migrations.AlterField( + model_name="service", + name="slug", + field=models.SlugField( + editable=False, + help_text=( + "Stable identifier used in the OpenSearch index name. " + "Lowercase alphanumeric only. Set on creation, immutable thereafter." + ), + max_length=20, + unique=True, + validators=[ + django.core.validators.RegexValidator( + message="Slug must contain only lowercase letters and digits.", + regex="^[a-z0-9]+$", + ) + ], + ), + ), + migrations.AlterField( + model_name="service", + name="name", + field=models.CharField(max_length=255), + ), + migrations.AddConstraint( + model_name="service", + constraint=models.CheckConstraint( + condition=models.Q(("slug__regex", "^[a-z0-9]+$")), + name="slug_alphanumeric_only", + ), + ), + ] diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 91bcae63..7f797391 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -1,16 +1,23 @@ """Models for find's core app""" +import re import secrets import string from django.contrib.auth.models import AbstractUser +from django.core.exceptions import ValidationError +from django.core.validators import RegexValidator from django.db import models from django.db.models.functions import Length -from django.utils.text import slugify from django.utils.translation import gettext_lazy as _ models.CharField.register_lookup(Length) TOKEN_LENGTH = 50 +SLUG_REGEX = r"^[a-z0-9]+$" +SLUG_VALIDATOR = RegexValidator( + regex=SLUG_REGEX, + message=_("Slug must contain only lowercase letters and digits."), +) class User(AbstractUser): @@ -20,15 +27,23 @@ class User(AbstractUser): class Service(models.Model): """Service registered to index its documents to our find""" - name = models.SlugField(max_length=20, unique=True) + name = models.CharField(max_length=255) + slug = models.SlugField( + max_length=20, + unique=True, + editable=False, + validators=[SLUG_VALIDATOR], + help_text=_( + "Stable identifier used in the OpenSearch index name. " + "Lowercase alphanumeric only. Set on creation, immutable thereafter." + ), + ) token = models.CharField(max_length=TOKEN_LENGTH) created_at = models.DateTimeField(auto_now_add=True) is_active = models.BooleanField(default=True) client_id = models.CharField(blank=True, null=True) services = models.ManyToManyField( - "self", - verbose_name=_("Allowed services for search"), - blank=True, + "self", blank=True, verbose_name=_("Allowed services for search") ) class Meta: @@ -41,14 +56,35 @@ class Meta: condition=models.Q(token__length=TOKEN_LENGTH), name="token_length_exact_50", ), + models.CheckConstraint( + condition=models.Q(slug__regex=SLUG_REGEX), + name="slug_alphanumeric_only", + ), ] def __str__(self): return self.name def save(self, *args, **kwargs): - """Automatically slugify the service name and generate a token on creation""" - self.name = slugify(self.name) + """Generate token, auto-derive slug on creation, enforce slug immutability. + + - ``slug`` is auto-derived from ``name`` if not provided (strip + non-alphanumeric, lowercase). It is immutable after creation. + - ``name`` is a free-form display field and can be edited. + - ``token`` is generated once on creation if missing. + """ + if not self.slug: + self.slug = re.sub(r"[^a-zA-Z0-9]", "", self.name or "").lower() + if self.pk is not None: + stored_slug = ( + Service.objects.filter(pk=self.pk) + .values_list("slug", flat=True) + .first() + ) + if self.slug != stored_slug: + raise ValidationError( + {"slug": _("Service.slug is immutable after creation.")} + ) if not self.token: self.token = self.generate_secure_token() super().save(*args, **kwargs) diff --git a/src/backend/core/services/indexing.py b/src/backend/core/services/indexing.py index 4098e5e1..a52a4a60 100644 --- a/src/backend/core/services/indexing.py +++ b/src/backend/core/services/indexing.py @@ -4,9 +4,10 @@ from django.conf import settings -from opensearchpy.exceptions import NotFoundError +from opensearchpy.exceptions import NotFoundError, RequestError from py3langid.langid import MODEL_FILE, LanguageIdentifier +from core.models import Service from core.services.opensearch_configuration import ( ANALYZERS, FILTERS, @@ -23,24 +24,44 @@ LANGUAGE_IDENTIFIER.set_languages(["en", "fr", "de", "nl"]) +def get_service_index_name(service_slug: str) -> str: + """Return the OpenSearch index name for a given service slug.""" + return f"{settings.OPENSEARCH_INDEX_PREFIX}-{service_slug}" + + +def get_all_active_service_indices() -> list[str]: + """Return index names for all currently active services.""" + return [ + get_service_index_name(s.slug) + for s in Service.objects.filter(is_active=True).only("slug") + ] + + def ensure_index_exists(index_name): """Create index if it does not exist""" + client = opensearch_client() try: - opensearch_client().indices.get(index=index_name) + client.indices.get(index=index_name) except NotFoundError: logger.info("Creating index: %s", index_name) - opensearch_client().indices.create( - index=index_name, - body={ - "settings": { - "analysis": { - "analyzer": ANALYZERS, - "filter": FILTERS, + try: + client.indices.create( + index=index_name, + body={ + "settings": { + "analysis": { + "analyzer": ANALYZERS, + "filter": FILTERS, + }, }, + "mappings": MAPPINGS, }, - "mappings": MAPPINGS, - }, - ) + ) + except RequestError as error: + if error.error == "resource_already_exists_exception": + pass # Another process created the index first — idempotent + else: + raise def prepare_document_for_indexing(document, service_name): diff --git a/src/backend/core/tests/conftest.py b/src/backend/core/tests/conftest.py index 0c1cd25f..b5f3f0d0 100644 --- a/src/backend/core/tests/conftest.py +++ b/src/backend/core/tests/conftest.py @@ -1,32 +1,29 @@ """Fixtures for tests in the find core application""" +import uuid + import pytest -from faker import Faker -from opensearchpy.exceptions import NotFoundError from core.services import opensearch -fake = Faker() - @pytest.fixture(autouse=True) def cleanup_test_index(settings): """ - Fixture to set a randomized index name for tests and remove it on tear down. + Fixture to set a unique index prefix per test and wipe all per-service indices + created under that prefix on teardown. """ - original_index = settings.OPENSEARCH_INDEX - test_index = "".join(fake.random_letters(5)).lower() - settings.OPENSEARCH_INDEX = test_index + unique = uuid.uuid4().hex[:12] + settings.OPENSEARCH_INDEX_PREFIX = f"test-{unique}" - # Create client here to prevent "teardown" issues when the opensearch settings are + # Create client here to prevent teardown issues when opensearch settings are # removed for error tests. client = opensearch.opensearch_client() yield - settings.OPENSEARCH_INDEX = original_index - - try: - client.indices.delete(index=test_index) - except NotFoundError: - pass + client.indices.delete( # pylint: disable=unexpected-keyword-arg + index=f"test-{unique}-*", + ignore_unavailable=True, + allow_no_indices=True, + ) diff --git a/src/backend/core/tests/test_api_documents_delete.py b/src/backend/core/tests/test_api_documents_delete.py index 3f6bbc69..0bcc3ea5 100644 --- a/src/backend/core/tests/test_api_documents_delete.py +++ b/src/backend/core/tests/test_api_documents_delete.py @@ -5,7 +5,6 @@ import opensearchpy import pytest -import responses from opensearchpy.helpers import bulk from rest_framework.test import APIClient @@ -13,7 +12,7 @@ from core.services.indexing import ensure_index_exists, prepare_document_for_indexing from core.services.opensearch import opensearch_client -from .utils import build_authorization_bearer, setup_oicd_resource_server +from .utils import build_authorization_bearer logger = logging.getLogger(__name__) @@ -56,28 +55,39 @@ def test_api_documents_delete_anonymous(): format="json", ) - assert response.status_code == 401 + assert response.status_code == 403 assert response.json() == { "detail": "Authentication credentials were not provided." } -@responses.activate +def test_api_documents_delete_oidc_token_rejected(): + """OIDC-style bearer tokens should not be allowed to delete documents.""" + response = APIClient().post( + "/api/v1.0/documents/delete/", + {"document_ids": ["doc1"]}, + format="json", + HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + ) + + assert response.status_code == 403 + assert response.json() == {"detail": "Invalid token."} + + def test_api_documents_delete_success(settings): - """Authenticated users should be able to delete documents they have access to.""" - setup_oicd_resource_server(responses, settings, sub="user_sub") + """Authenticated services should delete documents from their own index.""" service = factories.ServiceFactory() - # Create documents user has access to documents = factories.DocumentFactory.build_batch(3, users=["user_sub"]) - prepare_index(settings.OPENSEARCH_INDEX, documents, service_name=service.name) + service_index = f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}" + prepare_index(service_index, documents, service_name=service.slug) document_to_delete_ids = [doc["id"] for doc in documents[:2]] response = APIClient().post( "/api/v1.0/documents/delete/", {"document_ids": document_to_delete_ids}, format="json", - HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", ) assert response.status_code == 200 @@ -87,24 +97,19 @@ def test_api_documents_delete_success(settings): for document in documents: if document["id"] in document_to_delete_ids: with pytest.raises(opensearchpy.exceptions.NotFoundError): - opensearch_client_.get( - index=settings.OPENSEARCH_INDEX, id=document["id"] - ) + opensearch_client_.get(index=service_index, id=document["id"]) else: - doc = opensearch_client_.get( - index=settings.OPENSEARCH_INDEX, id=document["id"] - ) + doc = opensearch_client_.get(index=service_index, id=document["id"]) assert doc["found"] -@responses.activate -def test_api_documents_delete_no_access(settings): - """Users should not be able to delete documents they don't have access to.""" - setup_oicd_resource_server(responses, settings, sub="user_sub") +def test_api_documents_delete_other_service_index_untouched(settings): + """A service token should not delete documents from another service index.""" service = factories.ServiceFactory() - # Create documents where user_sub does NOT have access - documents = factories.DocumentFactory.build_batch(2, users=["other_sub"]) - prepare_index(settings.OPENSEARCH_INDEX, documents, service_name=service.name) + other_service = factories.ServiceFactory() + documents = factories.DocumentFactory.build_batch(2) + other_service_index = f"{settings.OPENSEARCH_INDEX_PREFIX}-{other_service.slug}" + prepare_index(other_service_index, documents, service_name=other_service.slug) document_ids = [doc["id"] for doc in documents] @@ -112,7 +117,7 @@ def test_api_documents_delete_no_access(settings): "/api/v1.0/documents/delete/", {"document_ids": document_ids}, format="json", - HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", ) assert response.status_code == 200 @@ -121,74 +126,56 @@ def test_api_documents_delete_no_access(settings): "undeleted-document-ids": document_ids, } - # Verify documents not deleted opensearch_client_ = opensearch_client() for doc_id in document_ids: - doc = opensearch_client_.get(index=settings.OPENSEARCH_INDEX, id=doc_id) + doc = opensearch_client_.get(index=other_service_index, id=doc_id) assert doc["found"] -@responses.activate -def test_api_documents_delete_mixed_access(settings): - """Deleting a mix of owned and non-owned documents should only delete owned ones.""" - setup_oicd_resource_server(responses, settings, sub="user_sub") +def test_api_documents_delete_mixed_existing_and_missing_ids(settings): + """Deleting a mix of existing and missing IDs should report only missing IDs.""" service = factories.ServiceFactory() - # Create documents with different access - owned_documents = factories.DocumentFactory.build_batch(2, users=["user_sub"]) - other_documents = factories.DocumentFactory.build_batch(2, users=["other_user"]) - prepare_index( - settings.OPENSEARCH_INDEX, - owned_documents + other_documents, - service_name=service.name, - ) + documents = factories.DocumentFactory.build_batch(4) + service_index = f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}" + prepare_index(service_index, documents, service_name=service.slug) - owned_document_ids = [doc["id"] for doc in owned_documents] - other_document_ids = [doc["id"] for doc in other_documents] + existing_document_ids = [doc["id"] for doc in documents] non_existing_document_ids = ["non-existent-1", "non-existent-2"] response = APIClient().post( "/api/v1.0/documents/delete/", - { - "document_ids": owned_document_ids - + other_document_ids - + non_existing_document_ids, - }, + {"document_ids": existing_document_ids + non_existing_document_ids}, format="json", - HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", ) assert response.status_code == 200 assert response.json() == { - "nb-deleted-documents": 2, - "undeleted-document-ids": other_document_ids + non_existing_document_ids, + "nb-deleted-documents": 4, + "undeleted-document-ids": non_existing_document_ids, } - # Verify only owned documents are deleted opensearch_client_ = opensearch_client() - for document_id in owned_document_ids: + for document_id in existing_document_ids: with pytest.raises(opensearchpy.exceptions.NotFoundError): - opensearch_client_.get(index=settings.OPENSEARCH_INDEX, id=document_id) - - for document_id in other_document_ids: - document = opensearch_client_.get( - index=settings.OPENSEARCH_INDEX, id=document_id - ) - assert document["found"] + opensearch_client_.get(index=service_index, id=document_id) -@responses.activate def test_api_documents_delete_missing_document_ids_and_tags(settings): """Requests missing both document_ids and tags should return 400.""" - setup_oicd_resource_server(responses, settings, sub="user_sub") service = factories.ServiceFactory() - prepare_index(settings.OPENSEARCH_INDEX, [], service_name=service.name) + prepare_index( + f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}", + [], + service_name=service.slug, + ) response = APIClient().post( "/api/v1.0/documents/delete/", {}, format="json", - HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", ) assert response.status_code == 400 @@ -201,16 +188,15 @@ def test_api_documents_delete_missing_document_ids_and_tags(settings): ] -@responses.activate -def test_api_documents_delete_empty_document_ids(settings): +def test_api_documents_delete_empty_document_ids(): """Requests with empty document_ids and no tags should return 400.""" - setup_oicd_resource_server(responses, settings, sub="user_sub") + service = factories.ServiceFactory() response = APIClient().post( "/api/v1.0/documents/delete/", {"document_ids": []}, format="json", - HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", ) assert response.status_code == 400 @@ -223,16 +209,15 @@ def test_api_documents_delete_empty_document_ids(settings): ] -@responses.activate -def test_api_documents_delete_both_filters_empty(settings): +def test_api_documents_delete_both_filters_empty(): """Requests with both document_ids and tags empty should return 400.""" - setup_oicd_resource_server(responses, settings, sub="user_sub") + service = factories.ServiceFactory() response = APIClient().post( "/api/v1.0/documents/delete/", {"document_ids": [], "tags": []}, format="json", - HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", ) assert response.status_code == 400 @@ -245,22 +230,24 @@ def test_api_documents_delete_both_filters_empty(settings): ] -@responses.activate def test_api_documents_delete_nonexistent_documents(settings): """ Deleting non-existent documents should not raise an error and return the list of undeleted ids. """ - setup_oicd_resource_server(responses, settings, sub="user_sub") service = factories.ServiceFactory() # Create index but with no documents - prepare_index(settings.OPENSEARCH_INDEX, [], service_name=service.name) + prepare_index( + f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}", + [], + service_name=service.slug, + ) response = APIClient().post( "/api/v1.0/documents/delete/", {"document_ids": ["non-existent-id"]}, format="json", - HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", ) assert response.status_code == 200 @@ -270,13 +257,11 @@ def test_api_documents_delete_nonexistent_documents(settings): } -@responses.activate @pytest.mark.flaky( reruns=2, reason="OpenSearch index race condition under high parallelism" ) def test_api_documents_delete_by_single_tag(settings): - """Users should be able to delete documents by tags.""" - setup_oicd_resource_server(responses, settings, sub="user_sub") + """Services should be able to delete documents by tags.""" service = factories.ServiceFactory() document_to_deletes = [ @@ -292,19 +277,20 @@ def test_api_documents_delete_by_single_tag(settings): users=["user_sub"], tags=["keep-tag-1", "keep-tag-2"] ), factories.DocumentFactory.build(users=["user_sub"], tags=["keep-tag-1"]), - factories.DocumentFactory.build(users=["other_user_sub"], tags=["delete-tag"]), + factories.DocumentFactory.build(users=["other_user_sub"], tags=["keep-tag-3"]), ] + service_index = f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}" prepare_index( - settings.OPENSEARCH_INDEX, + service_index, document_to_deletes + document_to_keep, - service_name=service.name, + service_name=service.slug, ) response = APIClient().post( "/api/v1.0/documents/delete/", {"tags": ["delete-tag"]}, format="json", - HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", ) assert response.status_code == 200 @@ -313,17 +299,15 @@ def test_api_documents_delete_by_single_tag(settings): opensearch_client_ = opensearch_client() for document in document_to_deletes: with pytest.raises(opensearchpy.exceptions.NotFoundError): - opensearch_client_.get(index=settings.OPENSEARCH_INDEX, id=document["id"]) + opensearch_client_.get(index=service_index, id=document["id"]) for document in document_to_keep: - doc = opensearch_client_.get(index=settings.OPENSEARCH_INDEX, id=document["id"]) + doc = opensearch_client_.get(index=service_index, id=document["id"]) assert doc["found"] -@responses.activate def test_api_documents_delete_by_multiple_tags(settings): - """Users should be able to delete documents matching any of multiple tags.""" - setup_oicd_resource_server(responses, settings, sub="user_sub") + """Services should be able to delete documents matching any of multiple tags.""" service = factories.ServiceFactory() document_to_deletes = [ @@ -340,21 +324,20 @@ def test_api_documents_delete_by_multiple_tags(settings): users=["user_sub"], tags=["keep-tag-1", "keep-tag-2"] ), factories.DocumentFactory.build(users=["user_sub"], tags=["keep-tag-1"]), - factories.DocumentFactory.build( - users=["other_user_sub"], tags=["delete-tag-1"] - ), + factories.DocumentFactory.build(users=["other_user_sub"], tags=["keep-tag-3"]), ] + service_index = f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}" prepare_index( - settings.OPENSEARCH_INDEX, + service_index, document_to_deletes + document_to_keep, - service_name=service.name, + service_name=service.slug, ) response = APIClient().post( "/api/v1.0/documents/delete/", {"tags": ["delete-tag-1", "delete-tag-2"]}, format="json", - HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", ) assert response.status_code == 200 @@ -363,17 +346,15 @@ def test_api_documents_delete_by_multiple_tags(settings): opensearch_client_ = opensearch_client() for document in document_to_deletes: with pytest.raises(opensearchpy.exceptions.NotFoundError): - opensearch_client_.get(index=settings.OPENSEARCH_INDEX, id=document["id"]) + opensearch_client_.get(index=service_index, id=document["id"]) for document in document_to_keep: - doc = opensearch_client_.get(index=settings.OPENSEARCH_INDEX, id=document["id"]) + doc = opensearch_client_.get(index=service_index, id=document["id"]) assert doc["found"] -@responses.activate def test_api_documents_delete_by_ids_and_tags(settings): - """Users should be able to delete documents by both IDs and tags (AND logic).""" - setup_oicd_resource_server(responses, settings, sub="user_sub") + """Services should be able to delete documents by both IDs and tags (AND logic).""" service = factories.ServiceFactory() document_delete_by_tag_and_id = factories.DocumentFactory.build( @@ -386,14 +367,15 @@ def test_api_documents_delete_by_ids_and_tags(settings): users=["user_sub"] ) + service_index = f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}" prepare_index( - settings.OPENSEARCH_INDEX, + service_index, [ document_delete_by_tag_and_id, document_delete_by_tag_keep_by_id, document_keep_by_tag_delete_by_id, ], - service_name=service.name, + service_name=service.slug, ) response = APIClient().post( @@ -406,7 +388,7 @@ def test_api_documents_delete_by_ids_and_tags(settings): "tags": ["delete-tag"], }, format="json", - HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", ) assert response.status_code == 200 @@ -418,10 +400,46 @@ def test_api_documents_delete_by_ids_and_tags(settings): opensearch_client_ = opensearch_client() with pytest.raises(opensearchpy.exceptions.NotFoundError): opensearch_client_.get( - index=settings.OPENSEARCH_INDEX, id=document_delete_by_tag_and_id["id"] + index=service_index, id=document_delete_by_tag_and_id["id"] ) doc = opensearch_client_.get( - index=settings.OPENSEARCH_INDEX, id=document_delete_by_tag_keep_by_id["id"] + index=service_index, id=document_delete_by_tag_keep_by_id["id"] ) assert doc["found"] + + +def test_api_documents_delete_by_tag_more_than_default_search_size(settings): + """Tag-only deletion should not be capped by OpenSearch's default 10 search hits.""" + service = factories.ServiceFactory() + document_to_deletes = factories.DocumentFactory.build_batch( + 12, users=["user_sub"], tags=["delete-tag"] + ) + document_to_keep = factories.DocumentFactory.build_batch( + 2, users=["user_sub"], tags=["keep-tag"] + ) + service_index = f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}" + prepare_index( + service_index, + document_to_deletes + document_to_keep, + service_name=service.slug, + ) + + response = APIClient().post( + "/api/v1.0/documents/delete/", + {"tags": ["delete-tag"]}, + format="json", + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", + ) + + assert response.status_code == 200 + assert response.json() == {"nb-deleted-documents": 12, "undeleted-document-ids": []} + + opensearch_client_ = opensearch_client() + for document in document_to_deletes: + with pytest.raises(opensearchpy.exceptions.NotFoundError): + opensearch_client_.get(index=service_index, id=document["id"]) + + for document in document_to_keep: + doc = opensearch_client_.get(index=service_index, id=document["id"]) + assert doc["found"] diff --git a/src/backend/core/tests/test_api_documents_index.py b/src/backend/core/tests/test_api_documents_index.py new file mode 100644 index 00000000..e4dc6662 --- /dev/null +++ b/src/backend/core/tests/test_api_documents_index.py @@ -0,0 +1,25 @@ +"""Authentication contract tests for the /index/ endpoint. + +These tests lock in the rule that only registered services can index documents: +/index/ is wired to ServiceTokenAuthentication, not ResourceServerAuthentication. +""" + +import pytest +from rest_framework.test import APIClient + +from .utils import build_authorization_bearer + +pytestmark = pytest.mark.django_db + + +def test_api_documents_index_oidc_token_rejected(): + """OIDC-style bearer tokens must not be accepted by /index/.""" + response = APIClient().post( + "/api/v1.0/documents/index/", + [], + format="json", + HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + ) + + assert response.status_code == 403 + assert response.json() == {"detail": "Invalid token."} diff --git a/src/backend/core/tests/test_api_documents_index_bulk.py b/src/backend/core/tests/test_api_documents_index_bulk.py index 9fa9cc55..7abe7cbd 100644 --- a/src/backend/core/tests/test_api_documents_index_bulk.py +++ b/src/backend/core/tests/test_api_documents_index_bulk.py @@ -70,7 +70,9 @@ def test_api_documents_index_bulk_ensure_index(): documents = factories.DocumentFactory.build_batch(3) with pytest.raises(NotFoundError): - opensearch_client_.indices.get(index=settings.OPENSEARCH_INDEX) + opensearch_client_.indices.get( + index=f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}" + ) response = APIClient().post( "/api/v1.0/documents/index/", @@ -86,8 +88,9 @@ def test_api_documents_index_bulk_ensure_index(): {"index": 2, "_id": documents[2]["id"], "status": "success"}, ] - # The index has been rebuilt - opensearch_client_.indices.get(index=settings.OPENSEARCH_INDEX) + opensearch_client_.indices.get( + index=f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}" + ) @pytest.mark.parametrize( @@ -302,7 +305,8 @@ def test_api_documents_index_bulk_default(field, default_value): ] indexed_document = opensearch.opensearch_client().get( - index=settings.OPENSEARCH_INDEX, id=documents[0]["id"] + index=f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}", + id=documents[0]["id"], )["_source"] assert indexed_document[field] == default_value diff --git a/src/backend/core/tests/test_api_documents_search.py b/src/backend/core/tests/test_api_documents_search.py new file mode 100644 index 00000000..4faf543d --- /dev/null +++ b/src/backend/core/tests/test_api_documents_search.py @@ -0,0 +1,56 @@ +"""Authentication contract tests for the /search/ endpoint. + +These tests lock in the rule that services cannot search: /search/ is wired to +ResourceServerAuthentication (OIDC users only), not ServiceTokenAuthentication. +A valid Service.token accepted by /index/ and /delete/ must be rejected here. +""" + +import json + +import pytest +import responses as responses_lib +from rest_framework.test import APIClient + +from core import factories + +from .utils import setup_oicd_resource_server + +pytestmark = pytest.mark.django_db + + +@responses_lib.activate +def test_api_documents_search_service_token_rejected(settings): + """Service bearer tokens must not be accepted by /search/.""" + setup_oicd_resource_server( + responses_lib, + settings, + introspect=lambda request, user_info: ( + 200, + {}, + json.dumps({"active": False}), + ), + ) + service = factories.ServiceFactory() + + response = APIClient().post( + "/api/v1.0/documents/search/", + {"q": "anything"}, + HTTP_AUTHORIZATION=f"Bearer {service.token}", + format="json", + ) + + assert response.status_code in (400, 401, 403) + + +@responses_lib.activate +def test_api_documents_search_anonymous_rejected(settings): + """Anonymous requests (no Authorization header) must be rejected by /search/.""" + setup_oicd_resource_server(responses_lib, settings) + + response = APIClient().post( + "/api/v1.0/documents/search/", + {"q": "anything"}, + format="json", + ) + + assert response.status_code in (400, 401, 403) diff --git a/src/backend/core/tests/test_fixture_isolation.py b/src/backend/core/tests/test_fixture_isolation.py new file mode 100644 index 00000000..17f4f710 --- /dev/null +++ b/src/backend/core/tests/test_fixture_isolation.py @@ -0,0 +1,37 @@ +"""Tests verifying the cleanup_test_index fixture behaviour.""" + +from django.conf import settings + +import pytest + +from core.services.indexing import ensure_index_exists +from core.services.opensearch import opensearch_client + +pytestmark = pytest.mark.django_db + + +def test_cleanup_test_index_wipes_per_service_indices(): + """Wildcard teardown removes all per-service indices created under the test prefix.""" + client = opensearch_client() + prefix = settings.OPENSEARCH_INDEX_PREFIX + + for svc in ("svc-a", "svc-b"): + ensure_index_exists(f"{prefix}-{svc}") + client.index( # pylint: disable=unexpected-keyword-arg + index=f"{prefix}-{svc}", + id="doc-1", + body={"service": svc, "title.en": "test doc"}, + refresh=True, + ) + + assert client.indices.exists(index=f"{prefix}-svc-a") + assert client.indices.exists(index=f"{prefix}-svc-b") + + client.indices.delete( # pylint: disable=unexpected-keyword-arg + index=f"{prefix}-*", + ignore_unavailable=True, + allow_no_indices=True, + ) + + assert not client.indices.exists(index=f"{prefix}-svc-a") + assert not client.indices.exists(index=f"{prefix}-svc-b") diff --git a/src/backend/core/tests/test_integration_per_service_indices.py b/src/backend/core/tests/test_integration_per_service_indices.py new file mode 100644 index 00000000..b845ffb5 --- /dev/null +++ b/src/backend/core/tests/test_integration_per_service_indices.py @@ -0,0 +1,286 @@ +"""Integration tests for the per-service OpenSearch index architecture. + +These tests exercise the full cross-cutting behaviour of the index-splitting +architecture: fan-out search, service-owned delete, service activation gating, +per-user access isolation, and Service.slug immutability. + +Service instances use auto-generated slugs (via ServiceFactory sequence) to +avoid unique-constraint collisions when the suite runs under pytest-xdist. +Index names are derived from the actual service.slug at runtime. +""" + +from django.core.exceptions import ValidationError + +import pytest +import responses as responses_lib +from rest_framework.test import APIClient + +from core import enums, factories +from core.services.indexing import ( + ensure_index_exists, + get_all_active_service_indices, + get_service_index_name, +) +from core.services.opensearch import opensearch_client +from core.services.search import search + +from .utils import build_authorization_bearer, setup_oicd_resource_server + +pytestmark = pytest.mark.django_db + + +def _index_via_api(service, documents): + """Index a list of documents through the service bearer-token API endpoint. + + Args: + service: A Service instance whose token authorises the request. + documents: A list of document dicts produced by DocumentFactory.build(). + """ + if not isinstance(documents, list): + documents = [documents] + response = APIClient().post( + "/api/v1.0/documents/index/", + documents, + HTTP_AUTHORIZATION=f"Bearer {service.token:s}", + format="json", + ) + assert response.status_code == 201 + + +def _refresh_index(index_name): + """Force an OpenSearch index refresh so newly indexed docs are searchable immediately. + + Args: + index_name: The fully-qualified OpenSearch index name to refresh. + """ + opensearch_client().indices.refresh(index=index_name) + + +@responses_lib.activate +def test_full_roundtrip_two_services(settings): + """Full roundtrip across two services: index, search, delete, confirm gone. + + - Index doc A via one service and doc B via another service. + - Fan-out search returns both docs. + - Each service deletes its own document. + - Follow-up search returns empty. + """ + user_sub = "roundtrip-user-sub" + setup_oicd_resource_server(responses_lib, settings, sub=user_sub) + + svc_a = factories.ServiceFactory() + svc_b = factories.ServiceFactory() + + doc_a = factories.DocumentFactory.build(users=[user_sub]) + doc_b = factories.DocumentFactory.build(users=[user_sub]) + + _index_via_api(svc_a, [doc_a]) + _index_via_api(svc_b, [doc_b]) + + idx_a = f"{settings.OPENSEARCH_INDEX_PREFIX}-{svc_a.slug}" + idx_b = f"{settings.OPENSEARCH_INDEX_PREFIX}-{svc_b.slug}" + _refresh_index(idx_a) + _refresh_index(idx_b) + + # Fan-out search — both docs must appear + search_resp = APIClient().post( + "/api/v1.0/documents/search/", + {"q": "*"}, + format="json", + HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + ) + assert search_resp.status_code == 200 + result_ids = [hit["_id"] for hit in search_resp.json()] + assert doc_a["id"] in result_ids + assert doc_b["id"] in result_ids + + # Service-owned delete — each service removes only its own doc + delete_resp_a = APIClient().post( + "/api/v1.0/documents/delete/", + {"document_ids": [doc_a["id"], doc_b["id"]]}, + format="json", + HTTP_AUTHORIZATION=f"Bearer {svc_a.token:s}", + ) + assert delete_resp_a.status_code == 200 + assert delete_resp_a.json() == { + "nb-deleted-documents": 1, + "undeleted-document-ids": [doc_b["id"]], + } + + delete_resp_b = APIClient().post( + "/api/v1.0/documents/delete/", + {"document_ids": [doc_b["id"]]}, + format="json", + HTTP_AUTHORIZATION=f"Bearer {svc_b.token:s}", + ) + assert delete_resp_b.status_code == 200 + assert delete_resp_b.json() == { + "nb-deleted-documents": 1, + "undeleted-document-ids": [], + } + + _refresh_index(idx_a) + _refresh_index(idx_b) + + # Follow-up search — both docs must be gone + search_resp2 = APIClient().post( + "/api/v1.0/documents/search/", + {"q": "*"}, + format="json", + HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + ) + assert search_resp2.status_code == 200 + result_ids2 = [hit["_id"] for hit in search_resp2.json()] + assert doc_a["id"] not in result_ids2 + assert doc_b["id"] not in result_ids2 + + +@responses_lib.activate +def test_deactivated_service_hidden_then_reactivated_visible(settings): + """Deactivating a service excludes it from search fan-out; reactivating restores it. + + - Index docX via a service — search finds docX. + - Set Service.is_active = False — search does NOT find docX (excluded from fan-out). + - Set Service.is_active = True — search finds docX again. + """ + user_sub = "archive-user-sub" + setup_oicd_resource_server(responses_lib, settings, sub=user_sub) + + svc = factories.ServiceFactory() + doc_x = factories.DocumentFactory.build(users=[user_sub]) + + _index_via_api(svc, [doc_x]) + svc_idx = f"{settings.OPENSEARCH_INDEX_PREFIX}-{svc.slug}" + _refresh_index(svc_idx) + + # Service active — docX must be visible + resp_active = APIClient().post( + "/api/v1.0/documents/search/", + {"q": "*"}, + format="json", + HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + ) + assert resp_active.status_code == 200 + assert doc_x["id"] in [hit["_id"] for hit in resp_active.json()] + + # Deactivate service — fan-out must skip this service's index + svc.is_active = False + svc.save() + + resp_inactive = APIClient().post( + "/api/v1.0/documents/search/", + {"q": "*"}, + format="json", + HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + ) + assert resp_inactive.status_code == 200 + assert doc_x["id"] not in [hit["_id"] for hit in resp_inactive.json()] + + # Reactivate service — docX must be visible again + svc.is_active = True + svc.save() + + resp_reactivated = APIClient().post( + "/api/v1.0/documents/search/", + {"q": "*"}, + format="json", + HTTP_AUTHORIZATION=f"Bearer {build_authorization_bearer()}", + ) + assert resp_reactivated.status_code == 200 + assert doc_x["id"] in [hit["_id"] for hit in resp_reactivated.json()] + + +def test_user_only_sees_own_docs_across_indices(settings): + """Cross-service user isolation: each user only sees documents they can access. + + - docA indexed in one service with restricted reach, only u1 in users. + - docB indexed in another service with restricted reach, only u2 in users. + - Searching as u1 returns docA and NOT docB. + - Searching as u2 returns docB and NOT docA. + """ + u1_sub = "isolation-user-one" + u2_sub = "isolation-user-two" + + svc_a = factories.ServiceFactory() + svc_b = factories.ServiceFactory() + + doc_a = factories.DocumentFactory.build( + users=[u1_sub], reach=enums.ReachEnum.RESTRICTED.value + ) + doc_b = factories.DocumentFactory.build( + users=[u2_sub], reach=enums.ReachEnum.RESTRICTED.value + ) + + _index_via_api(svc_a, [doc_a]) + _index_via_api(svc_b, [doc_b]) + + _refresh_index(f"{settings.OPENSEARCH_INDEX_PREFIX}-{svc_a.slug}") + _refresh_index(f"{settings.OPENSEARCH_INDEX_PREFIX}-{svc_b.slug}") + + indices = get_all_active_service_indices() + + # u1 must see docA only + result_u1 = search( + q="*", + nb_results=50, + order_by=enums.RELEVANCE, + order_direction="desc", + search_indices=indices, + reach=None, + visited=[], + user_sub=u1_sub, + groups=[], + tags=[], + ) + u1_ids = [hit["_id"] for hit in result_u1["hits"]["hits"]] + assert doc_a["id"] in u1_ids + assert doc_b["id"] not in u1_ids + + # u2 must see docB only + result_u2 = search( + q="*", + nb_results=50, + order_by=enums.RELEVANCE, + order_direction="desc", + search_indices=indices, + reach=None, + visited=[], + user_sub=u2_sub, + groups=[], + tags=[], + ) + u2_ids = [hit["_id"] for hit in result_u2["hits"]["hits"]] + assert doc_b["id"] in u2_ids + assert doc_a["id"] not in u2_ids + + +def test_service_slug_immutable_blocks_rename(settings): + """Service.slug is immutable: a re-slug attempt raises ValidationError. + + The original OpenSearch index is unaffected — its name is derived from the + immutable service slug, so the re-slug attempt cannot silently corrupt the + index topology. + + - Create a service and its OpenSearch index. + - Attempt to change the slug → ValidationError is raised. + - The original index still exists under its original name. + - No index under the attempted new slug was created. + """ + svc = factories.ServiceFactory() + original_index = get_service_index_name(svc.slug) + ensure_index_exists(original_index) + + client = opensearch_client() + assert client.indices.exists(index=original_index) + + # Attempt re-slug — must raise ValidationError + svc.slug = "immutablerenametgt" + with pytest.raises(ValidationError): + svc.save() + + # Original index must still exist unchanged + assert client.indices.exists(index=original_index) + + # No index must have been created for the attempted new slug + renamed_idx = f"{settings.OPENSEARCH_INDEX_PREFIX}-immutablerenametgt" + assert not client.indices.exists(index=renamed_idx) diff --git a/src/backend/core/tests/test_models_services.py b/src/backend/core/tests/test_models_services.py index f7bf6e3c..20c25c40 100644 --- a/src/backend/core/tests/test_models_services.py +++ b/src/backend/core/tests/test_models_services.py @@ -1,5 +1,6 @@ """Tests Service model for find's core app.""" +from django.core.exceptions import ValidationError from django.db import DataError, IntegrityError import pytest @@ -9,18 +10,37 @@ pytestmark = pytest.mark.django_db -def test_models_services_name_unique(): - """The name field should be unique across services.""" +def test_models_services_slug_unique(): + """The slug field must be unique across services.""" service = factories.ServiceFactory() with pytest.raises(IntegrityError): - factories.ServiceFactory(name=service.name) + factories.ServiceFactory(slug=service.slug) -def test_models_services_name_slugified(): - """The name field should be slugified.""" - service = factories.ServiceFactory(name="My service name") - assert service.name == "my-service-name" +def test_models_services_name_not_required_unique(): + """Two services may share the same display name.""" + factories.ServiceFactory(slug="aa", name="Same Name") + factories.ServiceFactory(slug="bb", name="Same Name") + + +def test_models_services_slug_auto_derived_from_name(): + """When no slug is provided, it is auto-derived from name (alphanumeric, lowercase).""" + service = factories.ServiceFactory(slug=None, name="My Service Name") + assert service.slug == "myservicename" + assert service.name == "My Service Name" + + +def test_models_services_slug_auto_derivation_strips_special_chars(): + """Slug derivation strips hyphens, underscores, spaces and punctuation.""" + service = factories.ServiceFactory(slug=None, name="docs-service_v2!") + assert service.slug == "docsservicev2" + + +def test_models_services_slug_rejects_non_alphanumeric(): + """Explicit non-alphanumeric slugs are rejected by the DB check constraint.""" + with pytest.raises(IntegrityError): + factories.ServiceFactory(slug="has-hyphen") def test_models_services_token_50_characters_exact(): @@ -39,3 +59,21 @@ def test_models_services_token_50_characters_more(): """The token field should be 50 characters long.""" with pytest.raises(DataError): factories.ServiceFactory(token="a" * 51) + + +def test_service_slug_immutable_after_creation(): + """The slug field must be immutable after creation.""" + service = factories.ServiceFactory(slug="originalname") + service.slug = "differentname" + with pytest.raises(ValidationError): + service.save() + + +def test_service_name_editable_after_creation(): + """The name field is freely editable after creation.""" + service = factories.ServiceFactory(slug="myslug", name="Original") + service.name = "New Display Name" + service.save() + service.refresh_from_db() + assert service.name == "New Display Name" + assert service.slug == "myslug" diff --git a/src/backend/core/tests/test_schemas.py b/src/backend/core/tests/test_schemas.py index bffa54fd..a60cddbd 100644 --- a/src/backend/core/tests/test_schemas.py +++ b/src/backend/core/tests/test_schemas.py @@ -2,7 +2,7 @@ import pytest -from core.schemas import cleanlist +from core.schemas import SearchQueryParameters, cleanlist def test_cleanlist_empty(): @@ -24,3 +24,8 @@ def test_cleanlist(): assert cleanlist(" 1, 2,3 ") == ["1", "2", "3"] assert cleanlist(["1 ", " 2", "3 "]) == ["1", "2", "3"] assert cleanlist([None, 2, 3, ""]) == ["2", "3"] + + +def test_search_query_parameters_no_services_field(): + """Assert that services field is not present in SearchQueryParameters""" + assert "services" not in SearchQueryParameters.model_fields.keys() diff --git a/src/backend/core/tests/test_service_isolation.py b/src/backend/core/tests/test_service_isolation.py index 6fcf88b1..470d84ea 100644 --- a/src/backend/core/tests/test_service_isolation.py +++ b/src/backend/core/tests/test_service_isolation.py @@ -17,8 +17,8 @@ class TestIndexServiceIsolation: def test_index_document_service_field_from_auth_not_payload(self): """Service field in payload is ignored; auth token determines service.""" - service = factories.ServiceFactory(name="docs-service") - document = factories.DocumentFactory.build(service="spoofed-drive") + service = factories.ServiceFactory(slug="docsservice") + document = factories.DocumentFactory.build(service="spoofeddrive") response = APIClient().post( "/api/v1.0/documents/index/", @@ -30,13 +30,15 @@ def test_index_document_service_field_from_auth_not_payload(self): assert response.status_code == status.HTTP_201_CREATED client = opensearch_client() - indexed_doc = client.get(index=settings.OPENSEARCH_INDEX, id=document["id"]) - assert indexed_doc["_source"]["service"] == "docs-service" + indexed_doc = client.get( + index=f"{settings.OPENSEARCH_INDEX_PREFIX}-docsservice", id=document["id"] + ) + assert indexed_doc["_source"]["service"] == "docsservice" def test_bulk_index_service_field_from_auth_not_payload(self): """Verify service field is correctly set for bulk indexing.""" - service = factories.ServiceFactory(name="my-docs") - documents = factories.DocumentFactory.build_batch(3, service="attempt-spoof") + service = factories.ServiceFactory(slug="mydocs") + documents = factories.DocumentFactory.build_batch(3, service="attemptspoof") response = APIClient().post( "/api/v1.0/documents/index/", @@ -49,5 +51,7 @@ def test_bulk_index_service_field_from_auth_not_payload(self): client = opensearch_client() for doc in documents: - indexed_doc = client.get(index=settings.OPENSEARCH_INDEX, id=doc["id"]) - assert indexed_doc["_source"]["service"] == "my-docs" + indexed_doc = client.get( + index=f"{settings.OPENSEARCH_INDEX_PREFIX}-mydocs", id=doc["id"] + ) + assert indexed_doc["_source"]["service"] == "mydocs" diff --git a/src/backend/core/tests/test_services_indexing.py b/src/backend/core/tests/test_services_indexing.py new file mode 100644 index 00000000..3a71f88e --- /dev/null +++ b/src/backend/core/tests/test_services_indexing.py @@ -0,0 +1,59 @@ +"""Tests for core.services.indexing helpers.""" + +from unittest.mock import MagicMock, patch + +import pytest +from opensearchpy.exceptions import NotFoundError, RequestError + +from core import factories +from core.services.indexing import ( + ensure_index_exists, + get_all_active_service_indices, + get_service_index_name, +) + +pytestmark = pytest.mark.django_db + + +@pytest.fixture(autouse=True) +def cleanup_test_index(): + """Override parent conftest fixture: indexing tests manage their own indices.""" + yield + + +def test_get_service_index_name_prefixes(settings): + """Index name is {prefix}-{service_slug}.""" + settings.OPENSEARCH_INDEX_PREFIX = "find" + assert get_service_index_name("docs") == "find-docs" + + +def test_get_all_active_service_indices_filters_inactive(settings): + """Only active services are included in the index list.""" + settings.OPENSEARCH_INDEX_PREFIX = "find" + factories.ServiceFactory(slug="svca", is_active=True) + factories.ServiceFactory(slug="svcb", is_active=True) + factories.ServiceFactory(slug="svcc", is_active=False) + indices = get_all_active_service_indices() + assert len(indices) == 2 + assert "find-svca" in indices + assert "find-svcb" in indices + + +def test_get_all_active_service_indices_empty(settings): + """Empty list returned when no active services exist.""" + settings.OPENSEARCH_INDEX_PREFIX = "find" + factories.ServiceFactory(is_active=False) + assert get_all_active_service_indices() == [] + + +def test_ensure_index_exists_idempotent_under_race(): + """ensure_index_exists handles race condition without raising.""" + mock_client = MagicMock() + mock_client.indices.get.side_effect = NotFoundError( + 404, "index_not_found_exception", {} + ) + mock_client.indices.create.side_effect = RequestError( + 400, "resource_already_exists_exception", {} + ) + with patch("core.services.indexing.opensearch_client", return_value=mock_client): + ensure_index_exists("test-idx") diff --git a/src/backend/core/views.py b/src/backend/core/views.py index da6c39e2..65fbe382 100644 --- a/src/backend/core/views.py +++ b/src/backend/core/views.py @@ -2,9 +2,6 @@ import logging -from django.conf import settings - -from lasuite.oidc_resource_server.authentication import ResourceServerAuthentication from lasuite.oidc_resource_server.mixins import ResourceServerMixin from pydantic import ValidationError as PydanticValidationError from rest_framework import status, views @@ -15,6 +12,8 @@ from .permissions import IsAuthAuthenticated from .services.indexing import ( ensure_index_exists, + get_all_active_service_indices, + get_service_index_name, prepare_document_for_indexing, ) from .services.opensearch import opensearch_client @@ -67,7 +66,7 @@ def post(self, request, *args, **kwargs): - Returns a list of results for all documents, with details of success and indexing errors. """ - index_name = settings.OPENSEARCH_INDEX + index_name = get_service_index_name(request.auth.slug) opensearch_client_ = opensearch_client() if isinstance(request.data, list): @@ -91,7 +90,7 @@ def single_index(self, request, index_name, opensearch_client_): """ document_dict = prepare_document_for_indexing( schemas.Document(**request.data).model_dump(), - service_name=request.auth.name, + service_name=request.auth.slug, ) _id = document_dict.pop("id") logger.info( @@ -143,7 +142,7 @@ def bulk_index(self, request, index_name, opensearch_client_): else: document_dict = prepare_document_for_indexing( document.model_dump(), - service_name=request.auth.name, + service_name=request.auth.slug, ) logger.info( "Indexing document %s on index %s", @@ -172,29 +171,27 @@ def bulk_index(self, request, index_name, opensearch_client_): return Response(results, status=status.HTTP_201_CREATED) -class DeleteDocumentsView(ResourceServerMixin, views.APIView): +class DeleteDocumentsView(views.APIView): """ API view for deleting documents from OpenSearch. - - Allows authenticated users to delete documents from a specified index. - - Users can only delete documents where they are listed in the 'users' field. + - Allows authenticated services to delete documents from their own index. + - Services can only delete documents from their isolated service index. - Returns the count of deleted documents without revealing document existence. """ - authentication_classes = [ResourceServerAuthentication] + authentication_classes = [ServiceTokenAuthentication] permission_classes = [IsAuthAuthenticated] def post(self, request, *args, **kwargs): """ - Handle POST requests to delete documents from the specified index. + Handle POST requests to delete documents from the authenticated service index. - Only documents where the authenticated user is in the 'users' field will be deleted. + A service can delete documents only from its own OpenSearch index. Body Parameters: --------------- - service: str - service name to determine the index from which to delete documents. document_ids : List[str], optional - A list of document IDs to delete from the index. + A list of document IDs to delete from the service index. tags : List[str], optional A list of tags to filter documents for deletion. @@ -208,7 +205,8 @@ def post(self, request, *args, **kwargs): - nb-deleted-documents: Number of documents deleted. - undeleted-document-ids: sublist of param.document_ids that were not deleted. Deletion may be prevented because the document does not exist, - because the user is not authorized to delete it or because a tag filter was used. + because it is outside the authenticated service index or because a tag filter + was used. - 400 Bad Request: If parameters are invalid or missing. """ params = schemas.DeleteDocuments(**request.data) @@ -219,23 +217,32 @@ def post(self, request, *args, **kwargs): params.tags, ) + index_name = get_service_index_name(request.auth.slug) client = opensearch_client() - deletable_matches = client.search( - index=settings.OPENSEARCH_INDEX, - body={ - "query": self._build_query( - self.request.user.sub, - document_ids=params.document_ids, - tags=params.tags, - ) - }, - ) - deletable_ids = [hit["_id"] for hit in deletable_matches["hits"]["hits"]] - if deletable_ids: - response = client.delete_by_query( - index=settings.OPENSEARCH_INDEX, - body={"query": {"ids": {"values": deletable_ids}}}, + if params.document_ids: + deletable_matches = client.search( # pylint: disable=unexpected-keyword-arg + index=index_name, + ignore_unavailable=True, + body={ + "size": len(params.document_ids), + "query": self._build_query( + document_ids=params.document_ids, + tags=params.tags, + ), + }, + ) + deletable_ids = [hit["_id"] for hit in deletable_matches["hits"]["hits"]] + delete_query = {"ids": {"values": deletable_ids}} + else: + deletable_ids = [] + delete_query = self._build_query(tags=params.tags) + + if delete_query != {"ids": {"values": []}}: + response = client.delete_by_query( # pylint: disable=unexpected-keyword-arg + index=index_name, + ignore_unavailable=True, + body={"query": delete_query}, ) nb_deleted = response.get("deleted", 0) else: @@ -253,21 +260,18 @@ def post(self, request, *args, **kwargs): status=status.HTTP_200_OK, ) - def _build_query(self, user_sub, document_ids=None, tags=None): + def _build_query(self, document_ids=None, tags=None): """ Build OpenSearch query for document deletion. Args: - user_sub: User subject identifier for authorization. document_ids: Optional list of document IDs to filter. tags: Optional list of tags to filter. Returns: Deletion OpenSearch query. """ - filters = [ - {"term": {"users": user_sub}}, - ] + filters = [] if document_ids: filters.append({"ids": {"values": document_ids}}) if tags: @@ -281,6 +285,7 @@ class SearchDocumentView(ResourceServerMixin, views.APIView): - Enables searching through indexed documents with support for various filters and sorting options. - The search results can be sorted or filtered via querystring parameters. + - Search is restricted to user OIDC tokens; service bearer tokens cannot search. """ permission_classes = [IsAuthAuthenticated] @@ -317,8 +322,6 @@ def post(self, request, *args, **kwargs): nb_results : int, optional The number of results to return. Defaults to 50 if not specified. - services: List[str], optional - List of services on which we intend to run the query (current service if left empty) visited: List[sub], optional List of public/authenticated documents the user has visited to limit the document returned to the ones the current user has seen. @@ -341,13 +344,18 @@ def post(self, request, *args, **kwargs): logger.error("Validation error: %s", errors) raise excpt - logger.info("Search '%s' on index %s", params.q, settings.OPENSEARCH_INDEX) + indices = get_all_active_service_indices() + + if not indices: + return Response([], status=status.HTTP_200_OK) + + logger.info("Search '%s' across %d service indices", params.q, len(indices)) result = search( q=params.q, nb_results=params.nb_results, order_by=params.order_by, order_direction=params.order_direction, - search_indices=[settings.OPENSEARCH_INDEX], + search_indices=indices, reach=params.reach, visited=params.visited, user_sub=user_sub, diff --git a/src/backend/demo/defaults.py b/src/backend/demo/defaults.py index d0115bf3..92721614 100644 --- a/src/backend/demo/defaults.py +++ b/src/backend/demo/defaults.py @@ -4,17 +4,20 @@ DEV_SERVICES = ( { - "name": "docs", + "slug": "docs", + "name": "Docs", "client_id": "impress", "token": "find-api-key-for-docs-with-exactly-50-chars-length", }, { - "name": "drive", + "slug": "drive", + "name": "Drive", "client_id": "drive", "token": "find-api-key-for-driv-with-exactly-50-chars-length", }, { - "name": "conversations", + "slug": "conversations", + "name": "Conversations", "client_id": "conversations", "token": "find-api-key-for-conv-with-exactly-50-chars-length", }, diff --git a/src/backend/demo/management/commands/create_demo.py b/src/backend/demo/management/commands/create_demo.py index 52c3f059..928bb8f8 100644 --- a/src/backend/demo/management/commands/create_demo.py +++ b/src/backend/demo/management/commands/create_demo.py @@ -146,7 +146,7 @@ def create_demo(stdout): """ opensearch_client_ = opensearch_client() try: - opensearch_client_.indices.delete(index="*") + opensearch_client_.indices.delete(index=f"{settings.OPENSEARCH_INDEX_PREFIX}-*") except NotFoundError: pass @@ -155,15 +155,18 @@ def create_demo(stdout): defaults.NB_OBJECTS["services"] ) - ensure_index_exists(settings.OPENSEARCH_INDEX) + for s in services: + ensure_index_exists(f"{settings.OPENSEARCH_INDEX_PREFIX}-{s.slug}") with Timeit(stdout, "Creating documents"): actions = BulkIndexing(stdout) for _ in range(defaults.NB_OBJECTS["documents"]): service = random.choice(services) document = generate_document() - document["service"] = service.name - actions.push(settings.OPENSEARCH_INDEX, uuid4(), document) + document["service"] = service.slug + actions.push( + f"{settings.OPENSEARCH_INDEX_PREFIX}-{service.slug}", uuid4(), document + ) actions.flush() with Timeit(stdout, "Creating dev services"): @@ -171,8 +174,11 @@ def create_demo(stdout): service = factories.ServiceFactory(**conf) # Check and report on indexed documents - opensearch_client_.indices.refresh(index=settings.OPENSEARCH_INDEX) - indexed = opensearch_client_.count(index=settings.OPENSEARCH_INDEX)["count"] + service_indices = ",".join( + f"{settings.OPENSEARCH_INDEX_PREFIX}-{s.slug}" for s in services + ) + opensearch_client_.indices.refresh(index=service_indices) + indexed = opensearch_client_.count(index=service_indices)["count"] stdout.write(f" TOTAL: {indexed:d} documents") diff --git a/src/backend/demo/tests/test_commands_create_demo.py b/src/backend/demo/tests/test_commands_create_demo.py index bd1169d6..a623ecea 100644 --- a/src/backend/demo/tests/test_commands_create_demo.py +++ b/src/backend/demo/tests/test_commands_create_demo.py @@ -26,11 +26,12 @@ def test_commands_create_demo(settings): """The create_demo management command should create objects as expected.""" call_command("create_demo") - assert models.Service.objects.exclude(name="docs").count() == 4 - assert opensearch_client().count(index=settings.OPENSEARCH_INDEX)["count"] == 4 + assert models.Service.objects.exclude(slug="docs").count() == 4 + index_pattern = f"{settings.OPENSEARCH_INDEX_PREFIX}-*" + assert opensearch_client().count(index=index_pattern)["count"] == 4 - docs = models.Service.objects.get(name="docs") + docs = models.Service.objects.get(slug="docs") assert docs.client_id == "impress" - drive = models.Service.objects.get(name="drive") + drive = models.Service.objects.get(slug="drive") assert drive.client_id == "drive" diff --git a/src/backend/find/settings.py b/src/backend/find/settings.py index 23668037..e8f094df 100755 --- a/src/backend/find/settings.py +++ b/src/backend/find/settings.py @@ -252,9 +252,9 @@ class Base(Configuration): OPENSEARCH_USE_SSL = values.BooleanValue( default=True, environ_name="OPENSEARCH_USE_SSL", environ_prefix=None ) - OPENSEARCH_INDEX = values.Value( - default="find", environ_name="OPENSEARCH_INDEX", environ_prefix=None - ) + OPENSEARCH_INDEX_PREFIX = values.Value( + default="find", environ_name="OPENSEARCH_INDEX_PREFIX", environ_prefix=None + ) # Per-service indices are named {OPENSEARCH_INDEX_PREFIX}-{service.slug} SPECTACULAR_SETTINGS = { "TITLE": "Find API",