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 @@ -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 @@ -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 @@ -163,7 +164,9 @@ """ - authentication_classes: Sequence[Type[BaseAuthentication]] = (BasicAuthentication,) + authentication_classes: Sequence[Type[BaseAuthentication]] = ( + KeycloakBasicAuthentication, + ) permission_classes: Sequence[Type[BasePermission]] = (IsAuthenticated,) 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,38 @@ -# 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 +import logging +from typing import Any, Dict, Optional + +from django.contrib.auth.models import User +import requests +from rest_framework import exceptions, status +from rest_framework.authentication import BasicAuthentication + +from swh.core.config import load_from_envvar +from swh.deposit.exception import ( + AuthenticationError, + AuthenticationMisconfigurationError, + BadRequestError, +) from .errors import UNAUTHORIZED, make_error_response +logger = logging.getLogger(__name__) + + +OIDC_DEPOSIT_CLIENT_ID = "swh-deposit" + +MAP_ERROR_CODE_TO_EXCEPTION = { + "invalid_grant": AuthenticationMisconfigurationError, + "unsupported_grant_type": AuthenticationMisconfigurationError, + "invalid_scope": AuthenticationMisconfigurationError, + "invalid_client": AuthenticationError, + "unauthorized_client": AuthenticationError, +} + def convert_response(request, content): """Convert response from drf's basic authentication mechanism to a @@ -61,3 +87,122 @@ return convert_response(request, response.content) return response + + +class KeycloakOpenIDConnectDirectGrant: + """Wrapper class around keycloak Direct Access Grant to ease keycloak interaction. + + Args: + auth_server: Keycloak authentication server url (e.g + https://auth.softwareheritage.org/auth/) + realm: The realm name (e.g SoftwareHeritage, SoftwareHeritageStaging, ...) + client_id: The OpenID connect client identifier in the associated keycloak realm + + """ + + def __init__( + self, auth_server: str, realm: str, client_id: str = OIDC_DEPOSIT_CLIENT_ID + ): + self.auth_server = auth_server.rstrip("/") + self.realm = realm + self.client_id = client_id + + def token_url(self): + """Compute the token url""" + return f"{self.auth_server}/realms/{self.realm}/protocol/openid-connect/token" + + def authenticate(self, username: str, password: str) -> Dict[str, Any]: + """Login and create new offline OpenID Connect session. + + Args: + username: an existing username in the realm + password: password associated to username + + Raises: + AuthenticationMisconfigurationError if admins misconfigured keycloak + AuthenticationError if users provided bad information + + Returns: + The OpenID Connect session info + + """ + response = requests.post( + url=self.token_url(), + data={ + "grant_type": "password", + "client_id": OIDC_DEPOSIT_CLIENT_ID, + "scope": "openid offline_access", + "username": username, + "password": password, + }, + ) + + if response.ok: + logger.info("Client %s logged in", username) + return response.json() + + try: + # try to parse some specific error details coming from keycloak if any + data = response.json() + except Exception: + # No specific error (e.g. not found, ...), plainly raise the error + response.raise_for_status() + + # Specific keycloak authentication issue + error_code = data["error"] + error_description = data.get("error_description", "") + exception_factory = MAP_ERROR_CODE_TO_EXCEPTION.get(error_code, BadRequestError) + logger.warning( + "Client %s failed authentication\nReason: %s - %s", + username, + error_code, + error_description, + ) + raise exception_factory(f"{error_code}: {error_description}") + + @classmethod + def from_config(cls, **kwargs: Any) -> "KeycloakOpenIDConnectDirectGrant": + cfg = kwargs["keycloak"] + return cls(auth_server=cfg["server_url"], realm=cfg["realm_name"]) + + @classmethod + def from_configfile(cls, **kwargs: Any) -> "KeycloakOpenIDConnectDirectGrant": + config = dict(load_from_envvar()) + return cls.from_config(**config) + + +class KeycloakBasicAuthentication(BasicAuthentication): + """Keycloack authentication against username/password. + + Deposit users will continue sending Basic authentication queries to our deposit + server. Transparently, the deposit server will stop authenticate itself the users + but delegates the authentication queries to the swh keycloak instance. + + Technically, reuses :class:`rest_framework.BasicAuthentication` and overrides the + funn:`authenticate_credentials` method to discuss with keycloak. + + """ + + _client: Optional[KeycloakOpenIDConnectDirectGrant] = None + + def authenticate_credentials(self, userid, password, request): + """Authenticate the userid/password against keycloak. + + """ + if self._client is None: + self._client = KeycloakOpenIDConnectDirectGrant.from_configfile() + + try: + _ = self._client.authenticate(userid, password) + except (AuthenticationMisconfigurationError, AuthenticationError) as e: + raise exceptions.AuthenticationFailed(e) + + try: + user = User.objects.get(username=userid) + except User.DoesNotExist: + raise exceptions.AuthenticationFailed(_("Invalid username/password.")) + + if not user.is_active: + raise exceptions.AuthenticationFailed(_("User inactive or deleted.")) + + return (user, 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="Desired user's 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,7 +113,8 @@ try: user = DepositClient.objects.get(username=username) # type: ignore click.echo(f"Update user '{username}'.") - user.set_password(password) + if password: + user.set_password(password) action_done = "updated" except DepositClient.DoesNotExist: click.echo(f"Create user '{username}'.") 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) @@ -35,3 +36,33 @@ return HttpResponse(data, status=503, content_type="application/xml") return response + + +class AuthenticationError(Exception): + """Authentication related error. + + Example: A bearer token has been revoked, client provided the wrong credentials, ... + + """ + + pass + + +class AuthenticationMisconfigurationError(Exception): + """Authentication misconfiguration related error. + + Example: A misconfigured client in keycloak, ... + + """ + + pass + + +class BadRequestError(Exception): + """Bad request coming from the client to forward. + + Example: A misconfigured client in keycloak, ... + + """ + + pass 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/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/conftest.py b/swh/deposit/tests/conftest.py --- a/swh/deposit/tests/conftest.py +++ b/swh/deposit/tests/conftest.py @@ -1,4 +1,4 @@ -# Copyright (C) 2019-2020 The Software Heritage developers +# Copyright (C) 2019-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 @@ -21,6 +21,7 @@ from swh.core.config import read from swh.core.pytest_plugin import get_response_cb +from swh.deposit.auth import KeycloakOpenIDConnectDirectGrant from swh.deposit.config import ( COL_IRI, DEPOSIT_STATUS_DEPOSITED, @@ -88,6 +89,10 @@ "checks": False, "scheduler": {"cls": "local", **swh_scheduler_config,}, "storage_metadata": swh_storage_backend_config, + "keycloak": { + "server_url": "https://auth.swh.org/SWHTest", + "realm_name": "SWHTest", + }, } @@ -201,7 +206,7 @@ user = DepositClient._default_manager.create_user( username=user_data["username"], email=user_data["email"], - password=user_data["password"], + # password=user_data["password"], provider_url=user_data["provider_url"], domain=user_data["domain"], ) @@ -245,12 +250,31 @@ @pytest.fixture -def authenticated_client(client, deposit_user): +def keycloak_access(): + return KeycloakOpenIDConnectDirectGrant( + "https://auth.swh.org/SWHTest", "SWHTest", "test-swh-deposit" + ) + + +@pytest.fixture +def keycloak_authorize_access(keycloak_access, requests_mock): + response = { + "access_token": "2YotnFZFEjr1zCsicMWpAA", + "token_type": "example", + "expires_in": 3600, + "refresh_token": "tGzv3JOkF0XG5Qx2TlKWIA", + "example_parameter": "example_value", + } + requests_mock.post(keycloak_access.token_url(), status_code=200, json=response) + + +@pytest.fixture +def authenticated_client(client, deposit_user, keycloak_authorize_access): yield from _create_authenticated_client(client, deposit_user, TEST_USER) @pytest.fixture -def another_authenticated_client(deposit_another_user): +def another_authenticated_client(deposit_another_user, keycloak_authorize_access): client = APIClient() yield from _create_authenticated_client( client, deposit_another_user, ANOTHER_TEST_USER diff --git a/swh/deposit/tests/test_auth.py b/swh/deposit/tests/test_auth.py new file mode 100644 --- /dev/null +++ b/swh/deposit/tests/test_auth.py @@ -0,0 +1,187 @@ +# 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 requests.exceptions import HTTPError + +from swh.core.config import read +from swh.deposit.auth import OIDC_DEPOSIT_CLIENT_ID, KeycloakOpenIDConnectDirectGrant +from swh.deposit.exception import ( + AuthenticationError, + AuthenticationMisconfigurationError, + BadRequestError, +) + + +@pytest.fixture() +def deposit_config(): + return { + "keycloak": { + "server_url": "https://auth.swh.org/SWHTest", + "realm_name": "SWHTest", + } + } + + +def test_auth_KeycloakOpenIDConnectDirectGrant_from_config(deposit_config): + """Instantiating the keycloak client out of configuration is fine + + """ + client = KeycloakOpenIDConnectDirectGrant.from_config(**deposit_config) + + assert client.auth_server == deposit_config["keycloak"]["server_url"] + assert client.realm == deposit_config["keycloak"]["realm_name"] + assert client.client_id == OIDC_DEPOSIT_CLIENT_ID + + +def test_auth_KeycloakOpenIDConnectDirectGrant_from_configfile( + deposit_config_path, monkeypatch +): + """Instantiating the keycloak client out of environment variable is fine as well + + """ + client = KeycloakOpenIDConnectDirectGrant.from_configfile() + + deposit_config = read(deposit_config_path) + + assert client.auth_server == deposit_config["keycloak"]["server_url"] + assert client.realm == deposit_config["keycloak"]["realm_name"] + assert client.client_id == OIDC_DEPOSIT_CLIENT_ID + + +def test_auth_authenticate_ok(requests_mock, keycloak_access): + response = { + "access_token": "2YotnFZFEjr1zCsicMWpAA", + "token_type": "example", + "expires_in": 3600, + "refresh_token": "tGzv3JOkF0XG5Qx2TlKWIA", + "example_parameter": "example_value", + } + requests_mock.post(keycloak_access.token_url(), status_code=200, json=response) + + actual_auth = keycloak_access.authenticate("user", "pass") + + assert actual_auth == response + + +@pytest.mark.parametrize( + "error,error_description", + [ + ( + "invalid_grant", + """The provided authorization grant (e.g., authorization + code, resource owner credentials) or refresh token is + invalid, expired, revoked, does not match the redirection + URI used in the authorization request, or was issued to + another client.""", + ), + ( + "unsupported_grant_type", + """The authorization grant type is not supported by the + authorization server.""", + ), + ( + "invalid_scope", + """The requested scope is invalid, unknown, malformed, or + exceeds the scope granted by the resource owner.""", + ), + ], +) +def test_auth_authenticate_misconfiguration( + requests_mock, error, error_description, keycloak_access +): + """Misconfigured keycloak should raise specific errors + + source: https://tools.ietf.org/html/rfc6749#section-5.2 + """ + requests_mock.post( + keycloak_access.token_url(), + status_code=400, + json={"error": error, "error_description": error_description}, + ) + + with pytest.raises(AuthenticationMisconfigurationError): + keycloak_access.authenticate("user", "pass") + + +@pytest.mark.parametrize( + "error,error_description", + [ + ( + "invalid_client", + """ + Client authentication failed (e.g., unknown client, no + client authentication included, or unsupported + authentication method). The authorization server MAY + return an HTTP 401 (Unauthorized) status code to indicate + which HTTP authentication schemes are supported. If the + client attempted to authenticate via the "Authorization" + request header field, the authorization server MUST + respond with an HTTP 401 (Unauthorized) status code and + include the "WWW-Authenticate" response header field + matching the authentication scheme used by the client.""", + ), + ( + "unauthorized_client", + """The authenticated client is not authorized to use this + authorization grant type.""", + ), + ], +) +def test_auth_authenticate_client_failure_connection( + requests_mock, error, error_description, keycloak_access +): + """Client failing with specific keycloak errors to authenticate should raise""" + requests_mock.post( + keycloak_access.token_url(), + status_code=400, + json={"error": error, "error_description": error_description}, + ) + + with pytest.raises(AuthenticationError): + keycloak_access.authenticate("user", "pass") + + +@pytest.mark.parametrize( + "error,error_description", + [ + ( + "invalid_request", + """The request is missing a required parameter, includes an + unsupported parameter value (other than grant type), + repeats a parameter, includes multiple credentials, + utilizes more than one mechanism for authenticating the + client, or is otherwise malformed""", + ), + ], +) +def test_auth_authenticate_bad_request( + requests_mock, error, error_description, keycloak_access +): + """Client sent a bad request with keycloak details, this should raise + + """ + requests_mock.post( + keycloak_access.token_url(), + status_code=400, + json={"error": error, "error_description": error_description}, + ) + + with pytest.raises(BadRequestError): + keycloak_access.authenticate("user", "pass") + + +@pytest.mark.parametrize("status_code", [400, 401, 404, 500]) +def test_auth_authenticate_unspecific_error( + requests_mock, status_code, keycloak_access +): + """Non-specific keycloak error should raise""" + + requests_mock.post( + keycloak_access.token_url(), status_code=status_code, + ) + + with pytest.raises(HTTPError): + keycloak_access.authenticate("user", "pass")