D5137.id18766.diff
No OneTemporary
Actions

Size

32 KB

Subscribers

None

D5137.id18766.diff
View Options

	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.2
	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
	@@ -17,13 +17,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 KeycloakBasicAuthentication
	from swh.deposit.models import Deposit
	from swh.deposit.utils import compute_metadata_context
	from swh.model import hashutil
	@@ -164,7 +165,9 @@

	"""

	- authentication_classes: Sequence[Type[BaseAuthentication]] = (BasicAuthentication,)
	+ authentication_classes: Sequence[Type[BaseAuthentication]] = (
	+ KeycloakBasicAuthentication,
	+ )
	permission_classes: Sequence[Type[BasePermission]] = (IsAuthenticated,)


	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,33 @@
	-# 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

	-from rest_framework import status
	+from datetime import datetime
	+import logging
	+from typing import Optional
	+
	+from django.core.cache import cache
	+from django.utils import timezone
	+from rest_framework import exceptions, status
	+from rest_framework.authentication import BasicAuthentication
	+import sentry_sdk
	+
	+from swh.auth.django.models import OIDCUser
	+
	+# from swh.auth.django.models import OIDCUser
	+from swh.auth.django.utils import oidc_user_from_decoded_token, 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"
	+REQUIRED_PERM = "swh.deposit.api"
	+

	def convert_response(request, content):
	"""Convert response from drf's basic authentication mechanism to a
	@@ -61,3 +82,89 @@
	return convert_response(request, response.content)

	return response
	+
	+
	+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: int) -> 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:
	+ sentry_sdk.capture_exception(e)
	+ return None
	+
	+ def authenticate_credentials(self, user_id, password, request):
	+ """Authenticate the user_id/password against keycloak.
	+
	+ """
	+ oidc_profile = self.get_user(user_id)
	+
	+ if not oidc_profile:
	+ try:
	+ oidc_profile = self.client.login(user_id, password)
	+ except Exception as e:
	+ raise exceptions.AuthenticationFailed(e)
	+
	+ decoded_token = self.client.decode_token(oidc_profile["access_token"])
	+
	+ # create OIDCUser from decoded token
	+ oidc_user = oidc_user_from_decoded_token(
	+ decoded_token, client_id=self.client.client_id
	+ )
	+
	+ # TODO: The permissions check should probably be moved elsewhere
	+ # Determine where first (django middleware maybe)
	+ if REQUIRED_PERM not in oidc_user.permissions:
	+ msg = (
	+ f"Insufficient permissions. {REQUIRED_PERM} not "
	+ f"in {oidc_user.permissions}"
	+ )
	+ raise exceptions.AuthenticationFailed(msg)
	+
	+ # Making sure the associated deposit client is correctly configured in backend
	+ try:
	+ deposit_client = DepositClient.objects.get(username=user_id)
	+ except DepositClient.DoesNotExist:
	+ raise exceptions.AuthenticationFailed(f"Unknown user {user_id}")
	+
	+ if not deposit_client.is_active:
	+ raise exceptions.AuthenticationFailed(f"Deactivated user {user_id}")
	+
	+ deposit_client = deposit_client.from_OIDCUser(oidc_user)
	+
	+ # cache the oidc_profile user while it's valid
	+ exp = datetime.fromtimestamp(decoded_token["exp"])
	+ ttl = int(exp.timestamp() - timezone.now().timestamp())
	+ cache.set(
	+ "oidc_user_{user_id}", deposit_client.to_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
	@@ -34,7 +34,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
	@@ -7,13 +7,17 @@
	# cd swh_deposit && \
	# python3 -m manage inspectdb

	+from copy import copy
	import datetime
	+from typing import Dict

	-from django.contrib.auth.models import User, UserManager
	+from django.contrib.auth.models import 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,
	@@ -80,7 +84,7 @@
	}


	-class DepositClient(User):
	+class DepositClient(OIDCUser):
	"""Deposit client

	"""
	@@ -95,6 +99,7 @@

	class Meta:
	db_table = "deposit_client"
	+ app_label = "deposit"

	def __str__(self):
	return str(
	@@ -107,6 +112,30 @@
	}
	)

	+ def save(self):
	+ super(OIDCUser, self).save()
	+
	+ def from_OIDCUser(self, oidc_user: OIDCUser) -> "DepositClient":
	+ """Convert an oidc user into a DepositClient.
	+
	+ """
	+ assert self.username == oidc_user.username
	+ for key, val in oidc_user.__dict__.items():
	+ if key in ["_state", "password", "id"]:
	+ continue
	+ setattr(self, key, val)
	+ return self
	+
	+ def to_oidc_profile(self) -> Dict:
	+ """Convert deposit client into an oidc_profile dict.
	+
	+ """
	+ data = copy(self.__dict__)
	+ for key in ["_state", "password"]:
	+ data.pop(key, "None")
	+
	+ return data
	+

	class Deposit(models.Model):
	"""Deposit reception table
	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",
	}

	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,12 +13,12 @@
	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


	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,6 +20,7 @@
	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.config import (
	@@ -42,29 +44,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=["swh.deposit.api"],
	+)
	+
	+
	+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")
	@@ -89,6 +134,10 @@
	"checks": False,
	"scheduler": {"cls": "local", **swh_scheduler_config,},
	"storage_metadata": swh_storage_backend_config,
	+ "keycloak": {
	+ "server_url": KEYCLOAK_SERVER_URL,
	+ "realm_name": KEYCLOAK_REALM_NAME,
	+ },
	}


	@@ -178,7 +227,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)
	@@ -186,57 +235,79 @@
	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

	+ user_data_d = deepcopy(user_data)
	+
	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 = DepositClient.objects.get( # type: ignore
	+ username=user_data_d["username"]
	)
	- user.collections = [collection.id]
	+ except DepositClient.DoesNotExist:
	+ user_data_d.pop("collection", None)
	+ user = DepositClient(**user_data_d, collections=[collection.id])
	user.save()
	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
	+@pytest.fixture
	+def mock_keycloakopenidconnect_ok(mocker, keycloak_mock_auth_success):
	+ mock = mocker.patch("swh.deposit.auth.KeycloakOpenIDConnect")
	+ mock.from_configfile.return_value = keycloak_mock_auth_success
	+ return mock
	+
	+
	+@pytest.fixture
	+def mock_keycloakopenidconnect_ko(mocker, keycloak_mock_auth_failure):
	+ mock = mocker.patch("swh.deposit.auth.KeycloakOpenIDConnect")
	+ mock.from_configfile.return_value = keycloak_mock_auth_failure
	+ return mock
	+
	+
	+@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):
	+ """Returned 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)
	@@ -246,16 +317,8 @@


	@pytest.fixture
	-def authenticated_client(client, deposit_user):
	- yield from _create_authenticated_client(client, deposit_user, TEST_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 authenticated_client(mock_keycloakopenidconnect_ok, anonymous_client, deposit_user):
	+ yield from _create_authenticated_client(anonymous_client, deposit_user)


	@pytest.fixture
	@@ -293,8 +356,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,
	@@ -307,7 +388,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,85 @@
	+# 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_user_insufficient_permission(
	+ mocker, keycloak_mock_auth_success, deposit_user
	+):
	+ """Keycloak ok, db ok, user has insufficient permissions, this raises"""
	+ mock = mocker.patch("swh.deposit.auth.KeycloakOpenIDConnect")
	+ # Sets the wrong permissions, so the login pass, but the permission check fails
	+ keycloak_mock_auth_success.user_permissions = []
	+ mock.from_configfile.return_value = keycloak_mock_auth_success
	+
	+ backend = KeycloakBasicAuthentication()
	+ with pytest.raises(AuthenticationFailed, match="permissions"):
	+ backend.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
	+
	+ 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"

File Metadata

Mime Type: text/plain
Expires: Wed, Jul 2, 12:12 PM (2 d, 9 h ago)
Storage Engine: blob
Storage Format: Raw Data
Storage Handle: 3232894

D5137.id18766.diffNo OneTemporaryActions

D5137.id18766.diffView Options

File Metadata

Event Timeline

D5137.id18766.diff
No OneTemporary
Actions

D5137.id18766.diff
View Options