diff --git a/requirements-swh-server.txt b/requirements-swh-server.txt --- a/requirements-swh-server.txt +++ b/requirements-swh-server.txt @@ -2,3 +2,4 @@ swh.loader.core >= 0.0.71 swh.scheduler >= 0.7.0 swh.model >= 0.3.8 +swh.auth[django] >= 0.3.3 diff --git a/swh/deposit/api/common.py b/swh/deposit/api/common.py --- a/swh/deposit/api/common.py +++ b/swh/deposit/api/common.py @@ -18,13 +18,14 @@ from django.urls import reverse from django.utils import timezone from rest_framework import status -from rest_framework.authentication import BaseAuthentication, BasicAuthentication +from rest_framework.authentication import BaseAuthentication from rest_framework.permissions import BasePermission, IsAuthenticated from rest_framework.request import Request from rest_framework.views import APIView from swh.deposit.api.checks import check_metadata from swh.deposit.api.converters import convert_status_detail +from swh.deposit.auth import HasDepositPermission, KeycloakBasicAuthentication from swh.deposit.models import Deposit from swh.deposit.utils import compute_metadata_context from swh.model import hashutil @@ -169,8 +170,13 @@ """ - authentication_classes: Sequence[Type[BaseAuthentication]] = (BasicAuthentication,) - permission_classes: Sequence[Type[BasePermission]] = (IsAuthenticated,) + authentication_classes: Sequence[Type[BaseAuthentication]] = ( + KeycloakBasicAuthentication, + ) + permission_classes: Sequence[Type[BasePermission]] = ( + IsAuthenticated, + HasDepositPermission, + ) class APIBase(APIConfig, AuthenticatedAPIView, metaclass=ABCMeta): diff --git a/swh/deposit/api/service_document.py b/swh/deposit/api/service_document.py --- a/swh/deposit/api/service_document.py +++ b/swh/deposit/api/service_document.py @@ -1,4 +1,4 @@ -# Copyright (C) 2017-2020 The Software Heritage developers +# Copyright (C) 2017-2021 The Software Heritage developers # See the AUTHORS file at the top-level directory of this distribution # License: GNU General Public License version 3, or any later version # See top-level LICENSE file for more information @@ -6,20 +6,22 @@ from django.shortcuts import render from django.urls import reverse -from ..config import COL_IRI -from ..models import DepositClient, DepositCollection -from .common import ACCEPT_ARCHIVE_CONTENT_TYPES, ACCEPT_PACKAGINGS, APIBase +from swh.deposit.api.common import ( + ACCEPT_ARCHIVE_CONTENT_TYPES, + ACCEPT_PACKAGINGS, + APIBase, +) +from swh.deposit.config import COL_IRI +from swh.deposit.models import DepositCollection class ServiceDocumentAPI(APIBase): - def get(self, req, *args, **kwargs): - client = DepositClient.objects.get(username=req.user) - + def get(self, request, *args, **kwargs): + client = request.user collections = {} - for col_id in client.collections: col = DepositCollection.objects.get(pk=col_id) - col_uri = req.build_absolute_uri(reverse(COL_IRI, args=[col.name])) + col_uri = request.build_absolute_uri(reverse(COL_IRI, args=[col.name])) collections[col.name] = col_uri context = { @@ -29,5 +31,8 @@ "collections": collections, } return render( - req, "deposit/service_document.xml", context, content_type="application/xml" + request, + "deposit/service_document.xml", + context, + content_type="application/xml", ) diff --git a/swh/deposit/auth.py b/swh/deposit/auth.py --- a/swh/deposit/auth.py +++ b/swh/deposit/auth.py @@ -1,12 +1,32 @@ -# Copyright (C) 2017 The Software Heritage developers +# Copyright (C) 2017-2021 The Software Heritage developers # See the AUTHORS file at the top-level directory of this distribution # License: GNU General Public License version 3, or any later version # See top-level LICENSE file for more information +import logging +from typing import Optional + +from django.core.cache import cache +from django.utils import timezone from rest_framework import status +from rest_framework.authentication import BasicAuthentication +from rest_framework.exceptions import AuthenticationFailed +from rest_framework.permissions import BasePermission +from sentry_sdk import capture_exception + +from swh.auth.django.models import OIDCUser +from swh.auth.django.utils import oidc_user_from_profile +from swh.auth.keycloak import KeycloakOpenIDConnect +from swh.deposit.models import DepositClient from .errors import UNAUTHORIZED, make_error_response +logger = logging.getLogger(__name__) + + +OIDC_DEPOSIT_CLIENT_ID = "swh-deposit" +DEPOSIT_PERMISSION = "swh.deposit.api" + def convert_response(request, content): """Convert response from drf's basic authentication mechanism to a @@ -61,3 +81,93 @@ return convert_response(request, response.content) return response + + +class HasDepositPermission(BasePermission): + """Allows access to authenticated users with the DEPOSIT_PERMISSION. + + """ + + def has_permission(self, request, view): + assert isinstance(request.user, DepositClient) + return request.user.oidc_user.has_perm(DEPOSIT_PERMISSION) + + +class KeycloakBasicAuthentication(BasicAuthentication): + """Keycloack authentication against username/password. + + Deposit users will continue sending `Basic authentication` queries to the deposit + server. Transparently, the deposit server will stop authenticate itself the users. + It will delegate the authentication queries to the keycloak instance. + + Technically, reuses :class:`rest_framework.BasicAuthentication` and overrides the + func:`authenticate_credentials` method to discuss with keycloak. + + As an implementation detail, this also uses the django cache mechanism to avoid too + many authentication request to keycloak. + + """ + + _client: Optional[KeycloakOpenIDConnect] = None + + @property + def client(self): + if self._client is None: + self._client = KeycloakOpenIDConnect.from_configfile( + client_id=OIDC_DEPOSIT_CLIENT_ID + ) + return self._client + + def get_user(self, user_id: str) -> Optional[OIDCUser]: + """Retrieve user from cache if any. + + """ + oidc_profile = cache.get(f"oidc_user_{user_id}") + if oidc_profile: + try: + return oidc_user_from_profile(self.client, oidc_profile) + except Exception as e: + capture_exception(e) + return None + + def authenticate_credentials(self, user_id, password, request): + """Authenticate the user_id/password against keycloak. + + Raises: + AuthenticationFailed in case of authentication failure + + Returns: + Tuple of deposit_client, None. + + """ + oidc_user = self.get_user(user_id) + ttl: Optional[int] = None + if not oidc_user: + try: + oidc_profile = self.client.login(user_id, password) + except Exception as e: + raise AuthenticationFailed(e) + + oidc_user = oidc_user_from_profile(self.client, oidc_profile) + ttl = int( + oidc_user.refresh_expires_at.timestamp() - timezone.now().timestamp() + ) + + # Making sure the associated deposit client is correctly configured in backend + try: + deposit_client = DepositClient.objects.get(username=user_id) + except DepositClient.DoesNotExist: + raise AuthenticationFailed(f"Unknown user {user_id}") + + if not deposit_client.is_active: + raise AuthenticationFailed(f"Deactivated user {user_id}") + + deposit_client.oidc_user = oidc_user + + if ttl: + # cache the oidc_profile user while it's valid + cache.set( + f"oidc_user_{user_id}", oidc_profile, timeout=max(0, ttl), + ) + + return (deposit_client, None) diff --git a/swh/deposit/cli/admin.py b/swh/deposit/cli/admin.py --- a/swh/deposit/cli/admin.py +++ b/swh/deposit/cli/admin.py @@ -73,7 +73,7 @@ @user.command("create") @click.option("--username", required=True, help="User's name") -@click.option("--password", required=True, help="Desired user's password (plain).") +@click.option("--password", help="(Deprecated) Desired user password (plain).") @click.option("--firstname", default="", help="User's first name") @click.option("--lastname", default="", help="User's last name") @click.option("--email", default="", help="User's email") @@ -113,13 +113,11 @@ try: user = DepositClient.objects.get(username=username) # type: ignore click.echo(f"Update user '{username}'.") - user.set_password(password) action_done = "updated" except DepositClient.DoesNotExist: click.echo(f"Create user '{username}'.") - user = DepositClient.objects.create_user( # type: ignore - username=username, password=password - ) + user = DepositClient(username=username) + user.save() action_done = "created" user.collections = [collection_.id] diff --git a/swh/deposit/config.py b/swh/deposit/config.py --- a/swh/deposit/config.py +++ b/swh/deposit/config.py @@ -35,7 +35,6 @@ ARCHIVE_TYPE = "archive" METADATA_TYPE = "metadata" - AUTHORIZED_PLATFORMS = ["development", "production", "testing"] DEPOSIT_STATUS_REJECTED = "rejected" diff --git a/swh/deposit/exception.py b/swh/deposit/exception.py --- a/swh/deposit/exception.py +++ b/swh/deposit/exception.py @@ -1,4 +1,4 @@ -# Copyright (C) 2020 The Software Heritage developers +# Copyright (C) 2020-2021 The Software Heritage developers # See the AUTHORS file at the top-level directory of this distribution # License: GNU General Public License version 3, or any later version # See top-level LICENSE file for more information @@ -8,7 +8,6 @@ from django.db.utils import OperationalError from django.http import HttpResponse from rest_framework.exceptions import APIException -from rest_framework.views import exception_handler def custom_exception_handler( @@ -17,6 +16,8 @@ """Custom deposit exception handler to ensure consistent xml output """ + from rest_framework.views import exception_handler + # drf's default exception handler first, to get the standard error response response = exception_handler(exc, context) diff --git a/swh/deposit/models.py b/swh/deposit/models.py --- a/swh/deposit/models.py +++ b/swh/deposit/models.py @@ -1,4 +1,4 @@ -# Copyright (C) 2017-2020 The Software Heritage developers +# Copyright (C) 2017-2021 The Software Heritage developers # See the AUTHORS file at the top-level directory of this distribution # License: GNU General Public License version 3, or any later version # See top-level LICENSE file for more information @@ -8,12 +8,15 @@ # python3 -m manage inspectdb import datetime +from typing import Optional from django.contrib.auth.models import User, UserManager from django.contrib.postgres.fields import ArrayField, JSONField from django.db import models from django.utils.timezone import now +from swh.auth.django.models import OIDCUser + from .config import ( ARCHIVE_TYPE, DEPOSIT_STATUS_DEPOSITED, @@ -92,6 +95,7 @@ provider_url = models.TextField(null=False) domain = models.TextField(null=False) + oidc_user: Optional[OIDCUser] = None class Meta: db_table = "deposit_client" @@ -122,7 +126,7 @@ # collection concerned by the deposit collection = models.ForeignKey("DepositCollection", models.DO_NOTHING) # Deprecated: Deposit's external identifier - external_id = models.TextField() + external_id = models.TextField(null=True) # URL of the origin of this deposit, null if this is a metadata-only deposit origin_url = models.TextField(null=True) # Deposit client diff --git a/swh/deposit/settings/common.py b/swh/deposit/settings/common.py --- a/swh/deposit/settings/common.py +++ b/swh/deposit/settings/common.py @@ -103,9 +103,7 @@ STATIC_URL = "/static/" REST_FRAMEWORK = { - "DEFAULT_AUTHENTICATION_CLASSES": ( - "rest_framework.authentication.BasicAuthentication", - ), + "DEFAULT_AUTHENTICATION_CLASSES": ("swh.deposit.auth.KeycloakBasicAuthentication",), "EXCEPTION_HANDLER": "swh.deposit.exception.custom_exception_handler", } @@ -113,3 +111,5 @@ "django.core.files.uploadhandler.MemoryFileUploadHandler", "django.core.files.uploadhandler.TemporaryFileUploadHandler", ] + +CACHES = {"default": {"BACKEND": "django.core.cache.backends.locmem.LocMemCache",}} diff --git a/swh/deposit/settings/testing.py b/swh/deposit/settings/testing.py --- a/swh/deposit/settings/testing.py +++ b/swh/deposit/settings/testing.py @@ -1,4 +1,4 @@ -# Copyright (C) 2017 The Software Heritage developers +# Copyright (C) 2017-2021 The Software Heritage developers # See the AUTHORS file at the top-level directory of this distribution # License: GNU General Public License version 3, or any later version # See top-level LICENSE file for more information diff --git a/swh/deposit/tests/api/test_collection.py b/swh/deposit/tests/api/test_collection.py --- a/swh/deposit/tests/api/test_collection.py +++ b/swh/deposit/tests/api/test_collection.py @@ -13,15 +13,23 @@ from swh.deposit.parsers import parse_xml -def test_deposit_post_will_fail_with_401(client): +def test_deposit_post_will_fail_with_401(unauthorized_client): """Without authentication, endpoint refuses access with 401 response """ url = reverse(COL_IRI, args=["hal"]) - response = client.post(url) + response = unauthorized_client.post(url) assert response.status_code == status.HTTP_401_UNAUTHORIZED +def test_deposit_post_insufficient_permission(insufficient_perm_client): + """With connection ok but insufficient permission, endpoint refuses access""" + url = reverse(COL_IRI, args=["hal"]) + response = insufficient_perm_client.post(url) + assert response.status_code == status.HTTP_403_FORBIDDEN + assert b"permission" in response.content + + def test_access_to_another_user_collection_is_forbidden( authenticated_client, deposit_another_collection, deposit_user ): diff --git a/swh/deposit/tests/api/test_collection_add_to_origin.py b/swh/deposit/tests/api/test_collection_add_to_origin.py --- a/swh/deposit/tests/api/test_collection_add_to_origin.py +++ b/swh/deposit/tests/api/test_collection_add_to_origin.py @@ -13,7 +13,7 @@ from swh.deposit.parsers import parse_xml from swh.deposit.tests.common import post_atom -from ..conftest import create_deposit +from ..conftest import internal_create_deposit def test_add_deposit_with_add_to_origin( @@ -56,7 +56,6 @@ def test_add_deposit_add_to_origin_conflict( authenticated_client, - another_authenticated_client, deposit_collection, deposit_another_collection, atom_dataset, @@ -72,10 +71,9 @@ origin_url = deposit_another_user.provider_url + external_id # create a deposit for that other user, with the same slug - create_deposit( - another_authenticated_client, - deposit_another_collection.name, - sample_archive, + internal_create_deposit( + deposit_another_user, + deposit_another_collection, external_id, DEPOSIT_STATUS_LOAD_SUCCESS, ) diff --git a/swh/deposit/tests/api/test_collection_reuse_slug.py b/swh/deposit/tests/api/test_collection_reuse_slug.py --- a/swh/deposit/tests/api/test_collection_reuse_slug.py +++ b/swh/deposit/tests/api/test_collection_reuse_slug.py @@ -19,7 +19,7 @@ from swh.deposit.parsers import parse_xml from swh.deposit.tests.common import post_atom -from ..conftest import create_deposit +from ..conftest import internal_create_deposit def test_act_on_deposit_rejected_is_not_permitted( @@ -189,12 +189,11 @@ def test_add_deposit_external_id_conflict_no_parent( authenticated_client, - another_authenticated_client, deposit_collection, deposit_another_collection, atom_dataset, - sample_archive, deposit_user, + deposit_another_user, ): """Posting a deposit with an external_id conflicting with an external_id of a different client does not create a parent relationship @@ -204,10 +203,9 @@ origin_url = deposit_user.provider_url + external_id # create a deposit for that other user, with the same slug - other_deposit = create_deposit( - another_authenticated_client, - deposit_another_collection.name, - sample_archive, + other_deposit = internal_create_deposit( + deposit_another_user, + deposit_another_collection, external_id, DEPOSIT_STATUS_LOAD_SUCCESS, ) @@ -234,13 +232,12 @@ def test_add_deposit_external_id_conflict_with_parent( authenticated_client, - another_authenticated_client, deposit_collection, deposit_another_collection, completed_deposit, atom_dataset, - sample_archive, deposit_user, + deposit_another_user, ): """Posting a deposit with an external_id conflicting with an external_id of a different client creates a parent relationship with the deposit @@ -255,10 +252,9 @@ origin_url = deposit_user.provider_url + deposit.external_id # create a deposit for that other user, with the same slug - other_deposit = create_deposit( - another_authenticated_client, - deposit_another_collection.name, - sample_archive, + other_deposit = internal_create_deposit( + deposit_another_user, + deposit_another_collection, deposit.external_id, DEPOSIT_STATUS_LOAD_SUCCESS, ) diff --git a/swh/deposit/tests/api/test_exception.py b/swh/deposit/tests/api/test_exception.py --- a/swh/deposit/tests/api/test_exception.py +++ b/swh/deposit/tests/api/test_exception.py @@ -1,4 +1,4 @@ -# Copyright (C) 2020 The Software Heritage developers +# Copyright (C) 2020-2021 The Software Heritage developers # See the AUTHORS file at the top-level directory of this distribution # License: GNU General Public License version 3, or any later version # See top-level LICENSE file for more information @@ -43,7 +43,7 @@ fake_response = Response( exception=fake_exception, status=fake_exception.status_code ) - mock_exception_handler = mocker.patch("swh.deposit.exception.exception_handler") + mock_exception_handler = mocker.patch("rest_framework.views.exception_handler") mock_exception_handler.return_value = fake_response response = custom_exception_handler(fake_exception, {}) diff --git a/swh/deposit/tests/api/test_get_file.py b/swh/deposit/tests/api/test_get_file.py --- a/swh/deposit/tests/api/test_get_file.py +++ b/swh/deposit/tests/api/test_get_file.py @@ -14,7 +14,7 @@ def test_api_deposit_content_nominal( - client, complete_deposit, partial_deposit_only_metadata + authenticated_client, complete_deposit, partial_deposit_only_metadata ): """Retrieve information on deposit should return 200 response @@ -28,14 +28,16 @@ } url = reverse(CONT_FILE_IRI, args=[deposit.collection.name, deposit.id]) - response = client.get(url) + response = authenticated_client.get(url) assert response.status_code == status.HTTP_200_OK actual_deposit = dict(parse_xml(response.content)) del actual_deposit["swh:deposit_date"] assert set(actual_deposit.items()) >= set(expected_deposit.items()) -def test_api_deposit_content_unknown(client, complete_deposit, deposit_collection): +def test_api_deposit_content_unknown( + authenticated_client, complete_deposit, deposit_collection +): """Retrieve information on unknown deposit or collection should return 404 """ @@ -47,5 +49,5 @@ (complete_deposit.collection.name, complete_deposit.id + 10), ]: url = reverse(CONT_FILE_IRI, args=[collection, deposit_id]) - response = client.get(url) + response = authenticated_client.get(url) assert response.status_code == status.HTTP_404_NOT_FOUND diff --git a/swh/deposit/tests/api/test_service_document.py b/swh/deposit/tests/api/test_service_document.py --- a/swh/deposit/tests/api/test_service_document.py +++ b/swh/deposit/tests/api/test_service_document.py @@ -27,16 +27,16 @@ assert response.status_code == status.HTTP_401_UNAUTHORIZED -def test_service_document(authenticated_client, deposit_user): +def test_service_document(authenticated_client): """With authentication, service document list user's collection """ url = reverse(SD_IRI) response = authenticated_client.get(url) - check_response(response, deposit_user.username) + check_response(response, authenticated_client.deposit_client.username) -def test_service_document_with_http_accept_header(authenticated_client, deposit_user): +def test_service_document_with_http_accept_header(authenticated_client): """With authentication, with browser, sd list user's collection """ @@ -44,7 +44,7 @@ response = authenticated_client.get( url, HTTP_ACCEPT="text/html,application/xml;q=9,*/*,q=8" ) - check_response(response, deposit_user.username) + check_response(response, authenticated_client.deposit_client.username) def check_response(response, username): diff --git a/swh/deposit/tests/cli/test_admin.py b/swh/deposit/tests/cli/test_admin.py --- a/swh/deposit/tests/cli/test_admin.py +++ b/swh/deposit/tests/cli/test_admin.py @@ -179,7 +179,7 @@ assert user.is_active is True second_password = user.password assert second_password is not None - assert second_password != first_password, "Password should have changed" + assert second_password == first_password, "Password not changed (no longer used)" assert user.domain == "domain" assert user.provider_url == "http://some-provider.org" assert user.email == "user@org.org" diff --git a/swh/deposit/tests/conftest.py b/swh/deposit/tests/conftest.py --- a/swh/deposit/tests/conftest.py +++ b/swh/deposit/tests/conftest.py @@ -4,11 +4,12 @@ # See top-level LICENSE file for more information import base64 +from copy import deepcopy from functools import partial from io import BytesIO import os import re -from typing import Mapping +from typing import TYPE_CHECKING, Dict, Mapping from django.test.utils import setup_databases # type: ignore from django.urls import reverse_lazy as reverse @@ -19,8 +20,10 @@ from rest_framework.test import APIClient import yaml +from swh.auth.pytest_plugin import keycloak_mock_factory from swh.core.config import read from swh.core.pytest_plugin import get_response_cb +from swh.deposit.auth import DEPOSIT_PERMISSION from swh.deposit.config import ( COL_IRI, DEPOSIT_STATUS_DEPOSITED, @@ -42,29 +45,72 @@ from swh.model.identifiers import CoreSWHID, ObjectType, QualifiedSWHID from swh.scheduler import get_scheduler +if TYPE_CHECKING: + from swh.deposit.models import Deposit, DepositClient, DepositCollection + + # mypy is asked to ignore the import statement above because setup_databases # is not part of the d.t.utils.__all__ variable. +USERNAME = "test" +EMAIL = "test@example.org" +COLLECTION = "test" TEST_USER = { - "username": "test", - "password": "password", - "email": "test@example.org", + "username": USERNAME, + "password": "", + "email": EMAIL, "provider_url": "https://hal-test.archives-ouvertes.fr/", "domain": "archives-ouvertes.fr/", - "collection": {"name": "test"}, + "collection": {"name": COLLECTION}, +} + +USER_INFO = { + "name": USERNAME, + "email": EMAIL, + "email_verified": False, + "family_name": "", + "given_name": "", + "groups": [], + "preferred_username": USERNAME, + "sub": "ffffffff-bbbb-4444-aaaa-14f61e6b7200", } +USERNAME2 = "test2" +EMAIL2 = "test@example.org" +COLLECTION2 = "another-collection" -ANOTHER_TEST_USER = { - "username": "test2", - "password": "password2", - "email": "test@example2.org", +TEST_USER2 = { + "username": USERNAME2, + "password": "", + "email": EMAIL2, "provider_url": "https://hal-test.archives-ouvertes.example/", "domain": "archives-ouvertes.example/", - "collection": {"name": "another-collection"}, + "collection": {"name": COLLECTION2}, } +KEYCLOAK_SERVER_URL = "https://auth.swh.org/SWHTest" +KEYCLOAK_REALM_NAME = "SWHTest" +CLIENT_ID = "swh-deposit" + + +keycloak_mock_auth_success = keycloak_mock_factory( + server_url=KEYCLOAK_SERVER_URL, + realm_name=KEYCLOAK_REALM_NAME, + client_id=CLIENT_ID, + auth_success=True, + user_info=USER_INFO, + user_permissions=[DEPOSIT_PERMISSION], +) + + +keycloak_mock_auth_failure = keycloak_mock_factory( + server_url=KEYCLOAK_SERVER_URL, + realm_name=KEYCLOAK_REALM_NAME, + client_id=CLIENT_ID, + auth_success=False, +) + def pytest_configure(): setup_django_for("testing") @@ -91,6 +137,10 @@ "storage": swh_storage_backend_config, "storage_metadata": swh_storage_backend_config, "swh_authority_url": "http://deposit.softwareheritage.example/", + "keycloak": { + "server_url": KEYCLOAK_SERVER_URL, + "realm_name": KEYCLOAK_REALM_NAME, + }, } @@ -180,7 +230,7 @@ return collection -def deposit_collection_factory(collection_name=TEST_USER["collection"]["name"]): +def deposit_collection_factory(collection_name): @pytest.fixture def _deposit_collection(db, collection_name=collection_name): return create_deposit_collection(collection_name) @@ -188,57 +238,85 @@ return _deposit_collection -deposit_collection = deposit_collection_factory() -deposit_another_collection = deposit_collection_factory("another-collection") +deposit_collection = deposit_collection_factory(COLLECTION) +deposit_another_collection = deposit_collection_factory(COLLECTION2) -def _create_deposit_user(db, collection, user_data): +def _create_deposit_user( + collection: "DepositCollection", user_data: Dict +) -> "DepositClient": """Create/Return the test_user "test" """ from swh.deposit.models import DepositClient - try: - user = DepositClient._default_manager.get(username=user_data["username"]) - except DepositClient.DoesNotExist: - user = DepositClient._default_manager.create_user( - username=user_data["username"], - email=user_data["email"], - password=user_data["password"], - provider_url=user_data["provider_url"], - domain=user_data["domain"], - ) - user.collections = [collection.id] - user.save() + user_data_d = deepcopy(user_data) + user_data_d.pop("collection", None) + user, _ = DepositClient.objects.get_or_create( # type: ignore + username=user_data_d["username"], + defaults={**user_data_d, "collections": [collection.id]}, + ) return user @pytest.fixture def deposit_user(db, deposit_collection): - return _create_deposit_user(db, deposit_collection, TEST_USER) + return _create_deposit_user(deposit_collection, TEST_USER) @pytest.fixture def deposit_another_user(db, deposit_another_collection): - return _create_deposit_user(db, deposit_another_collection, ANOTHER_TEST_USER) + return _create_deposit_user(deposit_another_collection, TEST_USER2) @pytest.fixture -def client(): - """Override pytest-django one which does not work for djangorestframework. +def anonymous_client(): + """Create an anonymous client (no credentials during queries to the deposit) """ return APIClient() # <- drf's client -def _create_authenticated_client(client, user, user_data): - """Returned a logged client +def mock_keycloakopenidconnect(mocker, keycloak_mock): + """Mock swh.deposit.auth.KeycloakOpenIDConnect to return the keycloak_mock + + """ + mock = mocker.patch("swh.deposit.auth.KeycloakOpenIDConnect") + mock.from_configfile.return_value = keycloak_mock + return mock + + +@pytest.fixture +def mock_keycloakopenidconnect_ok(mocker, keycloak_mock_auth_success): + """Mock keycloak so it always accepts connection for user with the right + permissions + + """ + return mock_keycloakopenidconnect(mocker, keycloak_mock_auth_success) + + +@pytest.fixture +def mock_keycloakopenidconnect_ko(mocker, keycloak_mock_auth_failure): + """Mock keycloak so it always refuses connections.""" + return mock_keycloakopenidconnect(mocker, keycloak_mock_auth_failure) + + +@pytest.fixture +def unauthorized_client(anonymous_client, mock_keycloakopenidconnect_ko): + """Create an unauthorized client (will see their authentication fail) + + """ + return anonymous_client + + +def _create_authenticated_client(client, user): + """Return a client whose credentials will be proposed to the deposit server. This also patched the client instance to keep a reference on the associated deposit_user. """ - _token = "%s:%s" % (user.username, user_data["password"]) + _token = "%s:%s" % (user.username, "irrelevant-in-test-context") token = base64.b64encode(_token.encode("utf-8")) authorization = "Basic %s" % token.decode("utf-8") client.credentials(HTTP_AUTHORIZATION=authorization) @@ -248,16 +326,21 @@ @pytest.fixture -def authenticated_client(client, deposit_user): - yield from _create_authenticated_client(client, deposit_user, TEST_USER) +def authenticated_client(mock_keycloakopenidconnect_ok, anonymous_client, deposit_user): + yield from _create_authenticated_client(anonymous_client, deposit_user) @pytest.fixture -def another_authenticated_client(deposit_another_user): - client = APIClient() - yield from _create_authenticated_client( - client, deposit_another_user, ANOTHER_TEST_USER - ) +def insufficient_perm_client( + mocker, keycloak_mock_auth_success, anonymous_client, deposit_user +): + """keycloak accepts connection but client returned has no deposit permission, so access + is not allowed. + + """ + keycloak_mock_auth_success.user_permissions = [] + mock_keycloakopenidconnect(mocker, keycloak_mock_auth_success) + yield from _create_authenticated_client(anonymous_client, deposit_user) @pytest.fixture @@ -295,8 +378,26 @@ return data +def internal_create_deposit( + client: "DepositClient", + collection: "DepositCollection", + external_id: str, + status: str, +) -> "Deposit": + """Create a deposit for a given collection with internal tool + + """ + from swh.deposit.models import Deposit + + deposit = Deposit( + client=client, external_id=external_id, status=status, collection=collection + ) + deposit.save() + return deposit + + def create_deposit( - authenticated_client, + client, collection_name: str, sample_archive, external_id: str, @@ -309,7 +410,7 @@ url = reverse(COL_IRI, args=[collection_name]) # when response = post_archive( - authenticated_client, + client, url, sample_archive, HTTP_SLUG=external_id, diff --git a/swh/deposit/tests/test_backend.py b/swh/deposit/tests/test_backend.py new file mode 100644 --- /dev/null +++ b/swh/deposit/tests/test_backend.py @@ -0,0 +1,71 @@ +# Copyright (C) 2021 The Software Heritage developers +# See the AUTHORS file at the top-level directory of this distribution +# License: GNU General Public License version 3, or any later version +# See top-level LICENSE file for more information + +import pytest +from rest_framework.exceptions import AuthenticationFailed + +from swh.deposit.auth import KeycloakBasicAuthentication +from swh.deposit.tests.conftest import TEST_USER + +REQUEST_OBJECT = "request-unused" +PASSWORD = "some-deposit-pass" + + +@pytest.fixture +def backend_success(mock_keycloakopenidconnect_ok, deposit_config, db): + """Backend whose connection to keycloak will systematically succeed.""" + return KeycloakBasicAuthentication() + + +@pytest.fixture +def backend_failure(mock_keycloakopenidconnect_ko, deposit_config): + """Backend whose connection to keycloak will systematically fail.""" + return KeycloakBasicAuthentication() + + +def test_backend_authentication_refused(backend_failure): + with pytest.raises(AuthenticationFailed): + backend_failure.authenticate_credentials( + TEST_USER["username"], PASSWORD, REQUEST_OBJECT + ) + + +def test_backend_authentication_db_misconfigured(backend_success): + """Keycloak configured ok, backend db misconfigured (missing user), this raises""" + with pytest.raises(AuthenticationFailed, match="Unknown"): + backend_success.authenticate_credentials( + TEST_USER["username"], PASSWORD, REQUEST_OBJECT + ) + + +def test_backend_authentication_user_inactive(backend_success, deposit_user): + """Keycloak configured ok, backend db configured, user inactive, this raises""" + deposit_user.is_active = False + deposit_user.save() + + with pytest.raises(AuthenticationFailed, match="Deactivated"): + backend_success.authenticate_credentials( + deposit_user.username, PASSWORD, REQUEST_OBJECT + ) + + +def test_backend_authentication_ok(backend_success, deposit_user): + """Keycloak configured ok, backend db configured ok, user logs in + + """ + user0, _ = backend_success.authenticate_credentials( + deposit_user.username, PASSWORD, REQUEST_OBJECT + ) + + assert user0 is not None + + # A second authentication call should leverage the django cache feature. + + user1, _ = backend_success.authenticate_credentials( + deposit_user.username, PASSWORD, REQUEST_OBJECT + ) + assert user1 is not None + + assert user0 == user1, "Should have been retrieved from the cache"