From 4f4744846e5578851fb36284756bd766a884179e Mon Sep 17 00:00:00 2001 From: npalaska Date: Mon, 6 Feb 2023 23:39:08 -0500 Subject: [PATCH 1/9] Use OIDC user for the functional test PBENCH-1070 --- lib/pbench/client/oidc_admin.py | 161 +++++++ lib/pbench/server/api/resources/__init__.py | 20 +- lib/pbench/server/auth/__init__.py | 2 +- lib/pbench/test/functional/server/conftest.py | 52 ++- .../test/functional/server/test_user.py | 428 +++++++++--------- .../test/unit/server/test_authorization.py | 6 +- .../unit/server/test_datasets_inventory.py | 3 +- .../unit/server/test_datasets_metadata.py | 9 +- .../etc/pbench-server/pbench-server.cfg | 2 +- 9 files changed, 427 insertions(+), 256 deletions(-) create mode 100644 lib/pbench/client/oidc_admin.py diff --git a/lib/pbench/client/oidc_admin.py b/lib/pbench/client/oidc_admin.py new file mode 100644 index 0000000000..6d60110848 --- /dev/null +++ b/lib/pbench/client/oidc_admin.py @@ -0,0 +1,161 @@ +import json +from typing import Dict, Optional, Union +from urllib.parse import urljoin + +import requests +from requests.structures import CaseInsensitiveDict + + +class OIDCAdmin: + def __init__( + self, + server_url: str, + headers: Optional[Dict[str, str]] = None, + verify: bool = False, + ): + self.server_url = server_url + self.headers = CaseInsensitiveDict({} if headers is None else headers) + self.verify = verify + self._connection = requests.Session() + + def _method( + self, + method: str, + path: str, + data: Union[Dict, str, None], + headers: Optional[Dict] = None, + raise_error: bool = True, + **kwargs, + ) -> requests.Response: + """Common frontend for the HTTP operations on OIDC client connection. + + Args: + method : The API HTTP method + path : Path for the request. + data : Json data to send with the request in case of the POST + headers : Optional headers to send with request + kwargs : Additional keyword args + + Returns: + Response from the request. + """ + final_headers = self.headers.copy() + if headers is not None: + final_headers.update(headers) + url = urljoin(self.server_url, path) + kwargs = dict( + params=kwargs, + data=data, + headers=final_headers, + verify=self.verify, + ) + response = self._connection.request(method, url, **kwargs) + if raise_error: + response.raise_for_status() + return response + + def get( + self, path: str, headers: Optional[Dict] = None, **kwargs + ) -> requests.Response: + """GET wrapper to handle an authenticated GET operation on the Resource + at a given path. + + Args: + path : Path for the request + headers : Additional headers to add to the request + kwargs : Additional keyword args to be added as URL parameters + + Returns: + Response from the request. + """ + return self._method("GET", path, None, headers=headers, **kwargs) + + def post( + self, + path: str, + data: Union[Dict, str], + headers: Optional[Dict] = None, + **kwargs, + ) -> requests.Response: + """POST wrapper to handle an authenticated POST operation on the + Resource at a given path. + + Args: + path : Path for the request + data : JSON request body + headers : Additional headers to add to the request + kwargs : Additional keyword args to be added as URL parameters + + Returns: + Response from the request. + """ + return self._method("POST", path, data, headers=headers, **kwargs) + + def get_admin_token(self) -> requests.Response: + url_path = "/realms/master/protocol/openid-connect/token" + data = { + "grant_type": "password", + "client_id": "admin-cli", + "username": "admin", + "password": "admin", + } + return self.post(path=url_path, data=data) + + def get_client_secret(self, client_id: str) -> requests.Response: + admin_token = self.get_admin_token().json().get("access_token") + url_path = f"admin/realms/pbench-server/clients?clientId={client_id}" + headers = { + "Content-Type": "application/json", + "Authorization": f"Bearer {admin_token}", + } + response = self.get(path=url_path, headers=headers) + return response.json()[0]["secret"] + + def create_new_user( + self, + username: str, + email: str, + password: str, + first_name: str = "", + last_name: str = "", + ) -> requests.Response: + admin_token = self.get_admin_token().json().get("access_token") + url_path = "/admin/realms/pbench-server/users" + headers = { + "Content-Type": "application/json", + "Authorization": f"Bearer {admin_token}", + } + data = { + "username": username, + "email": email, + "emailVerified": True, + "enabled": True, + "firstName": first_name, + "lastName": last_name, + "credentials": [ + {"type": "password", "value": password, "temporary": False} + ], + } + response = self.post(path=url_path, data=json.dumps(data), headers=headers) + return response + + def user_login( + self, client_id: str, username: str, password: str, client_secret: str = None + ) -> requests.Response: + url_path = "/realms/pbench-server/protocol/openid-connect/token" + headers = {"Content-Type": "application/x-www-form-urlencoded"} + client_secret = ( + client_secret + if client_secret + else self.get_client_secret(client_id=client_id) + ) + data = { + "client_id": client_id, + "grant_type": "password", + "client_secret": client_secret, + "scope": "profile email", + "username": username, + "password": password, + } + response = self.post(path=url_path, data=data, headers=headers) + return response diff --git a/lib/pbench/server/api/resources/__init__.py b/lib/pbench/server/api/resources/__init__.py index c2c81285f7..3e48574388 100644 --- a/lib/pbench/server/api/resources/__init__.py +++ b/lib/pbench/server/api/resources/__init__.py @@ -1231,13 +1231,6 @@ def _check_authorization(self, mode: ApiAuthorization): user_id = mode.user role = mode.role authorized_user: User = Auth.token_auth.current_user() - username = "none" - if user_id: - user = User.query(id=user_id) - if user: - username = user.username - else: - current_app.logger.error("User ID {} not found", user_id) # The ADMIN authorization doesn't involve a target resource owner or # access, so take care of that first as a special case. If there is @@ -1260,10 +1253,9 @@ def _check_authorization(self, mode: ApiAuthorization): access = mode.access current_app.logger.debug( - "Authorizing {} access for {} to user {} ({}) with access {} using {}", + "Authorizing {} access for {} to user (user id: {}) with access {} using {}", role, authorized_user, - username, user_id, mode.access, mode.type, @@ -1290,12 +1282,12 @@ def _check_authorization(self, mode: ApiAuthorization): # An unauthenticated user is never allowed to access private # data nor to perform an potential mutation of data: REJECT current_app.logger.warning( - "Attempt to {} user {} data without login", role, username + "Attempt to {} user {} data without login", role, user_id ) raise UnauthorizedAccess( authorized_user, role, - username, + user_id, access, HTTPStatus.UNAUTHORIZED, ) @@ -1308,7 +1300,7 @@ def _check_authorization(self, mode: ApiAuthorization): role, ) raise UnauthorizedAccess( - authorized_user, role, username, access, HTTPStatus.FORBIDDEN + authorized_user, role, user_id, access, HTTPStatus.FORBIDDEN ) elif ( user_id @@ -1322,10 +1314,10 @@ def _check_authorization(self, mode: ApiAuthorization): "Unauthorized attempt by {} to {} user {} data", authorized_user, role, - username, + user_id, ) raise UnauthorizedAccess( - authorized_user, role, username, access, HTTPStatus.FORBIDDEN + authorized_user, role, user_id, access, HTTPStatus.FORBIDDEN ) else: # We have determined that there is an authenticated user with diff --git a/lib/pbench/server/auth/__init__.py b/lib/pbench/server/auth/__init__.py index 072c984884..379c2adf9b 100644 --- a/lib/pbench/server/auth/__init__.py +++ b/lib/pbench/server/auth/__init__.py @@ -404,7 +404,7 @@ def token_introspect(self, token: str) -> JSON: token, self._pem_public_key, algorithms=[self._TOKEN_ALG], - audience=self.client_id, + audience=["account", self.client_id], options={ "verify_signature": True, "verify_aud": True, diff --git a/lib/pbench/test/functional/server/conftest.py b/lib/pbench/test/functional/server/conftest.py index 953afc4f93..01affaaaf3 100644 --- a/lib/pbench/test/functional/server/conftest.py +++ b/lib/pbench/test/functional/server/conftest.py @@ -3,7 +3,9 @@ import pytest -from pbench.client import API, PbenchServerClient +from pbench.client import PbenchServerClient +from pbench.client.oidc_admin import OIDCAdmin +from pbench.client.types import JSONMap @pytest.fixture(scope="module") @@ -25,19 +27,25 @@ def server_client(): @pytest.fixture(scope="module") -def register_test_user(server_client: PbenchServerClient): - """Create a test user for functional tests.""" +def oidc_admin(server_client: PbenchServerClient): + """ + Used by Pbench Server functional tests to get admin access + on OIDC server. + """ + oidc_endpoints = server_client.endpoints["openid-connect"] + oidc_server = OIDCAdmin(server_url=oidc_endpoints["issuer"]) + return oidc_server + - response = server_client.post( - API.REGISTER, - json={ - "username": "tester", - "first_name": "Test", - "last_name": "User", - "password": "123456", - "email": "tester@gmail.com", - }, - raise_error=False, +@pytest.fixture(scope="module") +def register_test_user(oidc_admin: OIDCAdmin): + """Create a test user for functional tests.""" + response = oidc_admin.create_new_user( + username="tester", + email="tester@gmail.com", + password="123456", + first_name="Test", + last_name="User", ) # To allow testing outside our transient CI containers, allow the tester @@ -48,9 +56,17 @@ def register_test_user(server_client: PbenchServerClient): @pytest.fixture -def login_user(server_client: PbenchServerClient, register_test_user): +def login_user( + server_client: PbenchServerClient, oidc_admin: OIDCAdmin, register_test_user +): """Log in the test user and return the authentication token""" - server_client.login("tester", "123456") - assert server_client.auth_token - yield - server_client.logout() + oidc_endpoints = server_client.endpoints["openid-connect"] + response = oidc_admin.user_login( + client_id=oidc_endpoints["client"], username="tester", password="123456" + ) + auth_token = response.json()["access_token"] + assert auth_token + json = {"username": "tester", "auth_token": auth_token} + server_client.username = "tester" + server_client.auth_token = auth_token + yield JSONMap(json) diff --git a/lib/pbench/test/functional/server/test_user.py b/lib/pbench/test/functional/server/test_user.py index a5ecd2b30e..5aef76a211 100644 --- a/lib/pbench/test/functional/server/test_user.py +++ b/lib/pbench/test/functional/server/test_user.py @@ -1,214 +1,214 @@ -from http import HTTPStatus - -import pytest -from requests import HTTPError - -from pbench.client import API, PbenchServerClient - -USERNAME1 = "functional" -USERNAME2 = "nonfunctional" -PASSWORD = "tests" - - -class TestUser: - """ - Pbench Server functional tests for basic "user" operations. - - NOTE: The functional test system is intended to run on a pristine - containerized Pbench Server. We could check for some pre-conditions if we - had a known Administrative user (e.g., admin/admin). - """ - - def test_login_fail(self, server_client: PbenchServerClient): - """ - Trying to log in to a non-existent user should fail. - - NOTE: This will fail if "nouser" exists. - """ - with pytest.raises( - HTTPError, - match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/login", - ): - server_client.login("nouser", "nopassword") - - @pytest.mark.parametrize( - "json", - [ - { - "username": USERNAME1, - "first_name": "Test", - "last_name": "Account", - "password": PASSWORD, - "email": f"{USERNAME1}@gmail.com", - }, - { - "username": USERNAME2, - "first_name": "Tester", - "last_name": "Accountant", - "password": PASSWORD, - "email": f"{USERNAME2}@gmail.com", - }, - ], - ) - def test_register(self, server_client: PbenchServerClient, json): - """ - Test that we can register a new user. - - NOTE: This will fail if the username already exists. - """ - response = server_client.post(API.REGISTER, json=json, raise_error=False) - assert ( - response.status_code == HTTPStatus.CREATED - ), f"Register failed with {response.json()}" - - def test_register_redux(self, server_client: PbenchServerClient): - """ - Test that an attempt to register an existing user fails. - """ - json = { - "username": USERNAME1, - "first_name": "Repeat", - "last_name": "Redux", - "password": PASSWORD, - "email": f"{USERNAME1}@gmail.com", - } - with pytest.raises( - HTTPError, - match=f"FORBIDDEN for url: {server_client.scheme}://.*?/api/v1/register", - ): - server_client.post(API.REGISTER, json=json) - - def test_profile_noauth(self, server_client: PbenchServerClient): - """ - Test that we can't access a user profile without authentication. - """ - with pytest.raises( - HTTPError, - match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME1}", - ): - server_client.get(API.USER, {"target_username": USERNAME1}) - - def test_login(self, server_client: PbenchServerClient): - """ - Test that we can log in using our new user. - - NOTE: This assumes test cases will be run in order. There are Pytest - plugins to explicitly control test case order, but Pytest generally - does run tests within a single class in order. Is it worth taking on - another dependency to make this explicit? - """ - server_client.login(USERNAME1, PASSWORD) - assert server_client.auth_token - - def test_profile_bad_auth(self, server_client: PbenchServerClient): - """ - Test that we can't access a user profile with an invalid authentication - token. - """ - with pytest.raises( - HTTPError, - match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME1}", - ): - server_client.get( - API.USER, - {"target_username": USERNAME1}, - headers={"Authorization": "Bearer of bad tokens"}, - ) - - def test_profile_not_self(self, server_client: PbenchServerClient): - """ - Test that we can't access another user's profile. - """ - with pytest.raises( - HTTPError, - match=f"FORBIDDEN for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME2}", - ): - server_client.get(API.USER, {"target_username": USERNAME2}) - - def test_profile(self, server_client: PbenchServerClient): - """ - Test that we can retrieve our own user profile once logged in. - - NOTE: This assumes test cases will be run in order. - """ - response = server_client.get(API.USER, {"target_username": USERNAME1}) - assert response.json()["username"] == USERNAME1 - - def test_update(self, server_client: PbenchServerClient): - """ - Test that we can update our own user profile once logged in. - - NOTE: This assumes test cases will be run in order. - """ - response = server_client.get(API.USER, {"target_username": USERNAME1}) - user = response.json() - assert user["first_name"] != "Mycroft" - user["first_name"] = "Mycroft" - - # Remove fields that PUT will reject, but use a copy so that we can - # compare the new value against what we expect. - # - # This is unfortunate: PUT should IGNORE unmodifiable fields to allow a - # standard REST GET/update/PUT sequence. - payload = user.copy() - del payload["registered_on"] - - response = server_client.put( - API.USER, {"target_username": USERNAME1}, json=payload - ) - put_response = response.json() - - response = server_client.get(API.USER, {"target_username": USERNAME1}) - updated = response.json() - assert user == updated - assert put_response == user - - def test_delete_other(self, server_client: PbenchServerClient): - """ - Test that we can't delete another user. - - NOTE: This assumes test cases will be run in order. - """ - with pytest.raises( - HTTPError, - match=f"FORBIDDEN for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME2}", - ): - server_client.delete(API.USER, {"target_username": USERNAME2}) - - def test_delete_nologin(self, server_client: PbenchServerClient): - """ - Test that we can't delete a user profile when not logged in. - - NOTE: This assumes test cases will be run in order. - """ - server_client.logout() - with pytest.raises( - HTTPError, - match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME1}", - ): - server_client.delete(API.USER, {"target_username": USERNAME1}) - - def test_delete(self, server_client: PbenchServerClient): - """ - Test that we can delete our own user profile once logged in. - - NOTE: This assumes test cases will be run in order. - """ - server_client.login(USERNAME1, PASSWORD) - server_client.delete(API.USER, {"target_username": USERNAME1}) - - def test_logout(self, server_client: PbenchServerClient): - """ - Test that we can log out from our session. - - NOTE: This assumes test cases will be run in order. - """ - server_client.logout() - assert server_client.auth_token is None - - def test_cleanup_user2(self, server_client: PbenchServerClient): - """ - Remove the second test user to make the test idempotent. - """ - server_client.login(USERNAME2, PASSWORD) - server_client.delete(API.USER, {"target_username": USERNAME2}) +# from http import HTTPStatus +# +# import pytest +# from requests import HTTPError +# +# from pbench.client import API, PbenchServerClient +# +# USERNAME1 = "functional" +# USERNAME2 = "nonfunctional" +# PASSWORD = "tests" +# +# +# class TestUser: +# """ +# Pbench Server functional tests for basic "user" operations. +# +# NOTE: The functional test system is intended to run on a pristine +# containerized Pbench Server. We could check for some pre-conditions if we +# had a known Administrative user (e.g., admin/admin). +# """ +# +# def test_login_fail(self, server_client: PbenchServerClient): +# """ +# Trying to log in to a non-existent user should fail. +# +# NOTE: This will fail if "nouser" exists. +# """ +# with pytest.raises( +# HTTPError, +# match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/login", +# ): +# server_client.login("nouser", "nopassword") +# +# @pytest.mark.parametrize( +# "json", +# [ +# { +# "username": USERNAME1, +# "first_name": "Test", +# "last_name": "Account", +# "password": PASSWORD, +# "email": f"{USERNAME1}@gmail.com", +# }, +# { +# "username": USERNAME2, +# "first_name": "Tester", +# "last_name": "Accountant", +# "password": PASSWORD, +# "email": f"{USERNAME2}@gmail.com", +# }, +# ], +# ) +# def test_register(self, server_client: PbenchServerClient, json): +# """ +# Test that we can register a new user. +# +# NOTE: This will fail if the username already exists. +# """ +# response = server_client.post(API.REGISTER, json=json, raise_error=False) +# assert ( +# response.status_code == HTTPStatus.CREATED +# ), f"Register failed with {response.json()}" +# +# def test_register_redux(self, server_client: PbenchServerClient): +# """ +# Test that an attempt to register an existing user fails. +# """ +# json = { +# "username": USERNAME1, +# "first_name": "Repeat", +# "last_name": "Redux", +# "password": PASSWORD, +# "email": f"{USERNAME1}@gmail.com", +# } +# with pytest.raises( +# HTTPError, +# match=f"FORBIDDEN for url: {server_client.scheme}://.*?/api/v1/register", +# ): +# server_client.post(API.REGISTER, json=json) +# +# def test_profile_noauth(self, server_client: PbenchServerClient): +# """ +# Test that we can't access a user profile without authentication. +# """ +# with pytest.raises( +# HTTPError, +# match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME1}", +# ): +# server_client.get(API.USER, {"target_username": USERNAME1}) +# +# def test_login(self, server_client: PbenchServerClient): +# """ +# Test that we can log in using our new user. +# +# NOTE: This assumes test cases will be run in order. There are Pytest +# plugins to explicitly control test case order, but Pytest generally +# does run tests within a single class in order. Is it worth taking on +# another dependency to make this explicit? +# """ +# server_client.login(USERNAME1, PASSWORD) +# assert server_client.auth_token +# +# def test_profile_bad_auth(self, server_client: PbenchServerClient): +# """ +# Test that we can't access a user profile with an invalid authentication +# token. +# """ +# with pytest.raises( +# HTTPError, +# match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME1}", +# ): +# server_client.get( +# API.USER, +# {"target_username": USERNAME1}, +# headers={"Authorization": "Bearer of bad tokens"}, +# ) +# +# def test_profile_not_self(self, server_client: PbenchServerClient): +# """ +# Test that we can't access another user's profile. +# """ +# with pytest.raises( +# HTTPError, +# match=f"FORBIDDEN for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME2}", +# ): +# server_client.get(API.USER, {"target_username": USERNAME2}) +# +# def test_profile(self, server_client: PbenchServerClient): +# """ +# Test that we can retrieve our own user profile once logged in. +# +# NOTE: This assumes test cases will be run in order. +# """ +# response = server_client.get(API.USER, {"target_username": USERNAME1}) +# assert response.json()["username"] == USERNAME1 +# +# def test_update(self, server_client: PbenchServerClient): +# """ +# Test that we can update our own user profile once logged in. +# +# NOTE: This assumes test cases will be run in order. +# """ +# response = server_client.get(API.USER, {"target_username": USERNAME1}) +# user = response.json() +# assert user["first_name"] != "Mycroft" +# user["first_name"] = "Mycroft" +# +# # Remove fields that PUT will reject, but use a copy so that we can +# # compare the new value against what we expect. +# # +# # This is unfortunate: PUT should IGNORE unmodifiable fields to allow a +# # standard REST GET/update/PUT sequence. +# payload = user.copy() +# del payload["registered_on"] +# +# response = server_client.put( +# API.USER, {"target_username": USERNAME1}, json=payload +# ) +# put_response = response.json() +# +# response = server_client.get(API.USER, {"target_username": USERNAME1}) +# updated = response.json() +# assert user == updated +# assert put_response == user +# +# def test_delete_other(self, server_client: PbenchServerClient): +# """ +# Test that we can't delete another user. +# +# NOTE: This assumes test cases will be run in order. +# """ +# with pytest.raises( +# HTTPError, +# match=f"FORBIDDEN for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME2}", +# ): +# server_client.delete(API.USER, {"target_username": USERNAME2}) +# +# def test_delete_nologin(self, server_client: PbenchServerClient): +# """ +# Test that we can't delete a user profile when not logged in. +# +# NOTE: This assumes test cases will be run in order. +# """ +# server_client.logout() +# with pytest.raises( +# HTTPError, +# match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME1}", +# ): +# server_client.delete(API.USER, {"target_username": USERNAME1}) +# +# def test_delete(self, server_client: PbenchServerClient): +# """ +# Test that we can delete our own user profile once logged in. +# +# NOTE: This assumes test cases will be run in order. +# """ +# server_client.login(USERNAME1, PASSWORD) +# server_client.delete(API.USER, {"target_username": USERNAME1}) +# +# def test_logout(self, server_client: PbenchServerClient): +# """ +# Test that we can log out from our session. +# +# NOTE: This assumes test cases will be run in order. +# """ +# server_client.logout() +# assert server_client.auth_token is None +# +# def test_cleanup_user2(self, server_client: PbenchServerClient): +# """ +# Remove the second test user to make the test idempotent. +# """ +# server_client.login(USERNAME2, PASSWORD) +# server_client.delete(API.USER, {"target_username": USERNAME2}) diff --git a/lib/pbench/test/unit/server/test_authorization.py b/lib/pbench/test/unit/server/test_authorization.py index d9a6ae3122..8639fa5ec3 100644 --- a/lib/pbench/test/unit/server/test_authorization.py +++ b/lib/pbench/test/unit/server/test_authorization.py @@ -70,7 +70,7 @@ def test_disallowed_admin(self, apibase, server_config, current_user_admin, ask) ApiAuthorizationType.USER_ACCESS, ask["role"], user, access ) ) - assert exc.value.owner == (ask["user"] if ask["user"] else "none") + assert exc.value.owner == (user if ask["user"] else None) assert exc.value.user == current_user_admin @pytest.mark.parametrize( @@ -128,7 +128,7 @@ def test_disallowed_auth( ApiAuthorizationType.USER_ACCESS, ask["role"], user, access ) ) - assert exc.value.owner == (ask["user"] if ask["user"] else "none") + assert exc.value.owner == (user if ask["user"] else None) assert exc.value.user == current_user_drb @pytest.mark.parametrize( @@ -176,7 +176,7 @@ def test_disallowed_noauth( ApiAuthorizationType.USER_ACCESS, ask["role"], user, access ) ) - assert exc.value.owner == (ask["user"] if ask["user"] else "none") + assert exc.value.owner == (user if ask["user"] else None) assert exc.value.user is None def test_admin_unauth(self, apibase, server_config, current_user_none): diff --git a/lib/pbench/test/unit/server/test_datasets_inventory.py b/lib/pbench/test/unit/server/test_datasets_inventory.py index 17bbce7126..d003149642 100644 --- a/lib/pbench/test/unit/server/test_datasets_inventory.py +++ b/lib/pbench/test/unit/server/test_datasets_inventory.py @@ -7,6 +7,7 @@ from pbench.server.cache_manager import CacheManager from pbench.server.database.models.datasets import Dataset, DatasetNotFound +from pbench.test.unit.server import TEST_USER_ID class TestDatasetsAccess: @@ -65,7 +66,7 @@ def test_dataset_not_present(self, query_get_as): def test_unauthorized_access(self, query_get_as): response = query_get_as("test", "metadata.log", HTTPStatus.FORBIDDEN) assert response.json == { - "message": "User drb is not authorized to READ a resource owned by test with private access" + "message": f"User drb is not authorized to READ a resource owned by {TEST_USER_ID} with private access" } def test_dataset_is_not_unpacked(self, query_get_as, monkeypatch): diff --git a/lib/pbench/test/unit/server/test_datasets_metadata.py b/lib/pbench/test/unit/server/test_datasets_metadata.py index eb3cd47bf5..9292c29215 100644 --- a/lib/pbench/test/unit/server/test_datasets_metadata.py +++ b/lib/pbench/test/unit/server/test_datasets_metadata.py @@ -6,6 +6,7 @@ from pbench.server import JSON, OperationCode from pbench.server.database.models.audit import Audit, AuditStatus, AuditType from pbench.server.database.models.datasets import Dataset, DatasetNotFound +from pbench.test.unit.server import DRB_USER_ID class TestDatasetsMetadataGet: @@ -163,7 +164,7 @@ def test_get_private_noauth(self, query_get_as): ) assert ( response.json["message"] - == "User test is not authorized to READ a resource owned by drb with private access" + == f"User test is not authorized to READ a resource owned by {DRB_USER_ID} with private access" ) def test_get_unauth(self, query_get_as): @@ -181,7 +182,7 @@ def test_get_unauth(self, query_get_as): ) assert ( response.json["message"] - == "Unauthenticated client is not authorized to READ a resource owned by drb with private access" + == f"Unauthenticated client is not authorized to READ a resource owned by {DRB_USER_ID} with private access" ) def test_get_bad_query(self, query_get_as): @@ -316,7 +317,7 @@ def test_put_nowrite(self, query_get_as, query_put_as): ) assert ( response.json["message"] - == "User test is not authorized to UPDATE a resource owned by drb with public access" + == f"User test is not authorized to UPDATE a resource owned by {DRB_USER_ID} with public access" ) def test_put_noauth(self, query_get_as, query_put_as): @@ -328,7 +329,7 @@ def test_put_noauth(self, query_get_as, query_put_as): ) assert ( response.json["message"] - == "Unauthenticated client is not authorized to UPDATE a resource owned by drb with public access" + == f"Unauthenticated client is not authorized to UPDATE a resource owned by {DRB_USER_ID} with public access" ) def test_put_invalid_name(self, query_get_as, query_put_as): diff --git a/server/pbenchinacan/etc/pbench-server/pbench-server.cfg b/server/pbenchinacan/etc/pbench-server/pbench-server.cfg index 068ffcc3d6..0b643a37e3 100644 --- a/server/pbenchinacan/etc/pbench-server/pbench-server.cfg +++ b/server/pbenchinacan/etc/pbench-server/pbench-server.cfg @@ -29,7 +29,7 @@ uri = postgresql://pbenchcontainer:pbench@localhost:5432/pbenchcontainer secret-key = "pbench-in-a-can secret shhh" [openid-connect] -#server_url = http://localhost:8090 +server_url = http://localhost:8090 secret = ########################################################################### From 77030699c53881e1a128bf9abf624e677e575dd8 Mon Sep 17 00:00:00 2001 From: npalaska Date: Sun, 12 Feb 2023 22:37:42 -0500 Subject: [PATCH 2/9] create a new scope in keycloak to add oidc client in aud claim --- lib/pbench/client/oidc_admin.py | 197 ++++++++---------- lib/pbench/server/auth/__init__.py | 10 +- lib/pbench/test/functional/server/conftest.py | 10 +- .../etc/pbench-server/pbench-server.cfg | 2 +- server/pbenchinacan/load_keycloak.sh | 39 +++- 5 files changed, 142 insertions(+), 116 deletions(-) diff --git a/lib/pbench/client/oidc_admin.py b/lib/pbench/client/oidc_admin.py index 6d60110848..89c715b6f1 100644 --- a/lib/pbench/client/oidc_admin.py +++ b/lib/pbench/client/oidc_admin.py @@ -1,97 +1,35 @@ +from http import HTTPStatus import json -from typing import Dict, Optional, Union -from urllib.parse import urljoin import requests -from requests.structures import CaseInsensitiveDict +from pbench.server.auth import Connection -class OIDCAdmin: - def __init__( - self, - server_url: str, - headers: Optional[Dict[str, str]] = None, - verify: bool = False, - ): - self.server_url = server_url - self.headers = CaseInsensitiveDict({} if headers is None else headers) - self.verify = verify - self._connection = requests.Session() - - def _method( - self, - method: str, - path: str, - data: Union[Dict, str, None], - headers: Optional[Dict] = None, - raise_error: bool = True, - **kwargs, - ) -> requests.Response: - """Common frontend for the HTTP operations on OIDC client connection. - Args: - method : The API HTTP method - path : Path for the request. - data : Json data to send with the request in case of the POST - headers : Optional headers to send with request - kwargs : Additional keyword args +class OIDCAdmin(Connection): + OIDC_REALM = "pbench-server" + OIDC_CLIENT = "pbench-dashboard" + ADMIN_USERNAME = "admin" + ADMIN_PASSWORD = "123" - Returns: - Response from the request. - """ - final_headers = self.headers.copy() - if headers is not None: - final_headers.update(headers) - url = urljoin(self.server_url, path) - kwargs = dict( - params=kwargs, - data=data, - headers=final_headers, - verify=self.verify, - ) - response = self._connection.request(method, url, **kwargs) - if raise_error: - response.raise_for_status() - return response - - def get( - self, path: str, headers: Optional[Dict] = None, **kwargs - ) -> requests.Response: - """GET wrapper to handle an authenticated GET operation on the Resource - at a given path. + def __init__(self, server_url: str): + super().__init__(server_url, verify=False) - Args: - path : Path for the request - headers : Additional headers to add to the request - kwargs : Additional keyword args to be added as URL parameters + def get_admin_token(self) -> dict: + """pbench-server realm admin user login. Returns: - Response from the request. - """ - return self._method("GET", path, None, headers=headers, **kwargs) - - def post( - self, - path: str, - data: Union[Dict, str], - headers: Optional[Dict] = None, - **kwargs, - ) -> requests.Response: - """POST wrapper to handle an authenticated POST operation on the - Resource at a given path. + access_token json payload - Args: - path : Path for the request - data : JSON request body - headers : Additional headers to add to the request - kwargs : Additional keyword args to be added as URL parameters + { 'access_token': , + 'expires_in': 60, 'refresh_expires_in': 1800, + 'refresh_token': , + 'token_type': 'Bearer', + 'not-before-policy': 0, + 'session_state': '8f558797-50e7-496d-bb45-3b5ac9fdcddb', + 'scope': 'profile email'} - Returns: - Response from the request. """ - return self._method("POST", path, data, headers=headers, **kwargs) - - def get_admin_token(self) -> requests.Response: url_path = "/realms/master/protocol/openid-connect/token" data = { "grant_type": "password", @@ -99,17 +37,7 @@ def get_admin_token(self) -> requests.Response: "username": "admin", "password": "admin", } - return self.post(path=url_path, data=data) - - def get_client_secret(self, client_id: str) -> requests.Response: - admin_token = self.get_admin_token().json().get("access_token") - url_path = f"admin/realms/pbench-server/clients?clientId={client_id}" - headers = { - "Content-Type": "application/json", - "Authorization": f"Bearer {admin_token}", - } - response = self.get(path=url_path, headers=headers) - return response.json()[0]["secret"] + return self.post(path=url_path, data=data).json() def create_new_user( self, @@ -119,8 +47,21 @@ def create_new_user( first_name: str = "", last_name: str = "", ) -> requests.Response: - admin_token = self.get_admin_token().json().get("access_token") - url_path = "/admin/realms/pbench-server/users" + """Creates a new user under the OIDC_REALM and assign OIDC_CLIENT_ROLE + to the new user. + + Args: + username: username to register, + email: user email address, + password: user password, + first_name: Optional first name of the user, + last_name: Optional first name of the user, + + Returns: + Response from the request. + """ + admin_token = self.get_admin_token().get("access_token") + url_path = f"/admin/realms/{self.OIDC_REALM}/users" headers = { "Content-Type": "application/json", "Authorization": f"Bearer {admin_token}", @@ -139,23 +80,69 @@ def create_new_user( response = self.post(path=url_path, data=json.dumps(data), headers=headers) return response - def user_login( - self, client_id: str, username: str, password: str, client_secret: str = None - ) -> requests.Response: - url_path = "/realms/pbench-server/protocol/openid-connect/token" + def user_login(self, client_id: str, username: str, password: str) -> dict: + """pbench-server realm user login on a specified client. + + Args: + client_id: client_name to use in the request + username: username of the user logging in + password: OIDC password + + Returns: + access_token json payload + + { 'access_token': , + 'expires_in': 60, 'refresh_expires_in': 1800, + 'refresh_token': , + 'token_type': 'Bearer', + 'not-before-policy': 0, + 'session_state': '8f558797-50e7-496d-bb45-3b5ac9fdcddb', + 'scope': 'profile email'} + + """ + url_path = f"/realms/{self.OIDC_REALM}/protocol/openid-connect/token" headers = {"Content-Type": "application/x-www-form-urlencoded"} - client_secret = ( - client_secret - if client_secret - else self.get_client_secret(client_id=client_id) - ) data = { "client_id": client_id, "grant_type": "password", - "client_secret": client_secret, "scope": "profile email", "username": username, "password": password, } response = self.post(path=url_path, data=data, headers=headers) - return response + return response.json() + + def get_user(self, username: str, token: str) -> dict: + """Get the OIDC user representation dict. + + Args: + username: username to query + token: access_token string to validate + + Returns: + User dict representation + + {'id': '37117992-a3de-43f7-b844-e6ee178e9965', + 'createdTimestamp': 1675981768951, + 'username': 'admin', + 'enabled': True, + 'totp': False, + 'emailVerified': False, + 'disableableCredentialTypes': [], + 'requiredActions': [], + 'notBefore': 0, + 'access': {'manageGroupMembership': True, 'view': True, 'mapRoles': True, 'impersonate': True, 'manage': True} + ... + } + """ + response = self.get( + f"admin/realms/{self.OIDC_REALM}/users?username={username}", + headers={ + "Content-Type": "application/json", + "Authorization": f"Bearer {token}", + }, + verify=False, + ) + if response.status_code == HTTPStatus.OK: + return response.json()[0] + return {} diff --git a/lib/pbench/server/auth/__init__.py b/lib/pbench/server/auth/__init__.py index 379c2adf9b..d3b5207b37 100644 --- a/lib/pbench/server/auth/__init__.py +++ b/lib/pbench/server/auth/__init__.py @@ -50,7 +50,7 @@ def _method( self, method: str, path: str, - data: Union[Dict, None], + data: Union[Dict, str, None], headers: Optional[Dict] = None, **kwargs, ) -> requests.Response: @@ -126,7 +126,11 @@ def get( return self._method("GET", path, None, headers=headers, **kwargs) def post( - self, path: str, data: Dict, headers: Optional[Dict] = None, **kwargs + self, + path: str, + data: Union[Dict, str], + headers: Optional[Dict] = None, + **kwargs, ) -> requests.Response: """POST wrapper to handle an authenticated POST operation on the Resource at a given path. @@ -404,7 +408,7 @@ def token_introspect(self, token: str) -> JSON: token, self._pem_public_key, algorithms=[self._TOKEN_ALG], - audience=["account", self.client_id], + audience=[self.client_id], options={ "verify_signature": True, "verify_aud": True, diff --git a/lib/pbench/test/functional/server/conftest.py b/lib/pbench/test/functional/server/conftest.py index 01affaaaf3..848bf661c8 100644 --- a/lib/pbench/test/functional/server/conftest.py +++ b/lib/pbench/test/functional/server/conftest.py @@ -32,8 +32,8 @@ def oidc_admin(server_client: PbenchServerClient): Used by Pbench Server functional tests to get admin access on OIDC server. """ - oidc_endpoints = server_client.endpoints["openid-connect"] - oidc_server = OIDCAdmin(server_url=oidc_endpoints["issuer"]) + oidc_endpoints = server_client.endpoints["openid"] + oidc_server = OIDCAdmin(server_url=oidc_endpoints["server"]) return oidc_server @@ -51,7 +51,7 @@ def register_test_user(oidc_admin: OIDCAdmin): # To allow testing outside our transient CI containers, allow the tester # user to already exist. assert ( - response.ok or response.status_code == HTTPStatus.FORBIDDEN + response.ok or response.status_code == HTTPStatus.CONFLICT ), f"Register failed with {response.json()}" @@ -60,11 +60,11 @@ def login_user( server_client: PbenchServerClient, oidc_admin: OIDCAdmin, register_test_user ): """Log in the test user and return the authentication token""" - oidc_endpoints = server_client.endpoints["openid-connect"] + oidc_endpoints = server_client.endpoints["openid"] response = oidc_admin.user_login( client_id=oidc_endpoints["client"], username="tester", password="123456" ) - auth_token = response.json()["access_token"] + auth_token = response["access_token"] assert auth_token json = {"username": "tester", "auth_token": auth_token} server_client.username = "tester" diff --git a/server/pbenchinacan/etc/pbench-server/pbench-server.cfg b/server/pbenchinacan/etc/pbench-server/pbench-server.cfg index 0b643a37e3..34e8a32b39 100644 --- a/server/pbenchinacan/etc/pbench-server/pbench-server.cfg +++ b/server/pbenchinacan/etc/pbench-server/pbench-server.cfg @@ -30,7 +30,7 @@ secret-key = "pbench-in-a-can secret shhh" [openid-connect] server_url = http://localhost:8090 -secret = +secret = keycloak_secret ########################################################################### # The rest will come from the default config file. diff --git a/server/pbenchinacan/load_keycloak.sh b/server/pbenchinacan/load_keycloak.sh index 56dfba4e6b..16c284ea16 100755 --- a/server/pbenchinacan/load_keycloak.sh +++ b/server/pbenchinacan/load_keycloak.sh @@ -65,10 +65,45 @@ else echo "Created ${REALM} realm" fi +# Create a client scope with custom mapper that a functional test +# user can include in the OIDC token request over a REST call. +# This will instruct Keycloak to include the CLIENT_ID (pbench-dashboard) +# when someone request a token Over a rest API using a CLIENT_ID. +# Having CLIENT_ID in the aud claim of the token is essential for the token +# to be validated. +curl -si -f -X POST "${KEYCLOAK_HOST_PORT}/admin/realms/${REALM}/client-scopes" \ + -H "Authorization: Bearer ${ADMIN_TOKEN}" \ + -H "Content-Type: application/json" \ + -d '{ + "name": "pbench", + "description": "", + "protocol": "openid-connect", + "attributes": { + "include.in.token.scope": "true", + "display.on.consent.screen": "true", + "gui.order": "", + "consent.screen.text": "" + }, + "protocolMappers": [ + { + "name": "pbench-mapper", + "protocol": "openid-connect", + "protocolMapper": "oidc-audience-mapper", + "consentRequired": false, + "config": { + "included.client.audience": "pbench-dashboard", + "id.token.claim": "false", + "access.token.claim": "true" + } + } + ] + }' + + CLIENT_CONF=$(curl -si -f -X POST "${KEYCLOAK_HOST_PORT}/admin/realms/${REALM}/clients" \ -H "Authorization: Bearer ${ADMIN_TOKEN}" \ -H "Content-Type: application/json" \ - -d '{"clientId": "'${CLIENT}'", "publicClient": true, "directAccessGrantsEnabled": true, "enabled": true, "redirectUris": ["'${KEYCLOAK_REDIRECT_URI}'"]}') + -d '{"clientId": "'${CLIENT}'", "publicClient": true, "defaultClientScopes": ["pbench", "openid", "profile", "email"], "directAccessGrantsEnabled": true, "serviceAccountsEnabled": true, "enabled": true, "redirectUris": ["'${KEYCLOAK_REDIRECT_URI}'"]}') CLIENT_ID=$(grep -o -e 'http://[^[:space:]]*' <<< ${CLIENT_CONF} | sed -e 's|.*/||') if [[ -z "${CLIENT_ID}" ]]; then @@ -124,7 +159,7 @@ else echo "Assigned an 'ADMIN' client role to the user 'admin' created above" fi -# Verify that the user id has a role assigned to it +# Verify that the user id has a role 'ADMIN' assigned to it USER_ROLES=$(curl -s "${KEYCLOAK_HOST_PORT}/admin/realms/${REALM}/users/${USER_ID}/role-mappings/clients/${CLIENT_ID}" \ -H "Authorization: Bearer ${ADMIN_TOKEN}" \ -H "Content-Type: application/json" | jq -r '.[].name') From c95f14998d7eb156c961c2d77ddf0ce50dda477f Mon Sep 17 00:00:00 2001 From: npalaska Date: Mon, 13 Feb 2023 13:19:08 -0500 Subject: [PATCH 3/9] Review comments --- lib/pbench/client/oidc_admin.py | 20 +++++++++++--------- server/pbenchinacan/load_keycloak.sh | 8 ++++---- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/lib/pbench/client/oidc_admin.py b/lib/pbench/client/oidc_admin.py index 89c715b6f1..84e68b5004 100644 --- a/lib/pbench/client/oidc_admin.py +++ b/lib/pbench/client/oidc_admin.py @@ -1,5 +1,6 @@ from http import HTTPStatus import json +import os import requests @@ -8,9 +9,8 @@ class OIDCAdmin(Connection): OIDC_REALM = "pbench-server" - OIDC_CLIENT = "pbench-dashboard" - ADMIN_USERNAME = "admin" - ADMIN_PASSWORD = "123" + ADMIN_USERNAME = os.getenv("ADMIN_USERNAME", "admin") + ADMIN_PASSWORD = os.getenv("ADMIN_PASSWORD", "admin") def __init__(self, server_url: str): super().__init__(server_url, verify=False) @@ -34,8 +34,8 @@ def get_admin_token(self) -> dict: data = { "grant_type": "password", "client_id": "admin-cli", - "username": "admin", - "password": "admin", + "username": self.ADMIN_USERNAME, + "password": self.ADMIN_PASSWORD, } return self.post(path=url_path, data=data).json() @@ -47,8 +47,10 @@ def create_new_user( first_name: str = "", last_name: str = "", ) -> requests.Response: - """Creates a new user under the OIDC_REALM and assign OIDC_CLIENT_ROLE - to the new user. + """Creates a new user under the OIDC_REALM. + + Note: This involves a REST API call to the + OIDC server to create a new user. Args: username: username to register, @@ -136,12 +138,12 @@ def get_user(self, username: str, token: str) -> dict: } """ response = self.get( - f"admin/realms/{self.OIDC_REALM}/users?username={username}", + f"admin/realms/{self.OIDC_REALM}/users", headers={ "Content-Type": "application/json", "Authorization": f"Bearer {token}", }, - verify=False, + username=username, ) if response.status_code == HTTPStatus.OK: return response.json()[0] diff --git a/server/pbenchinacan/load_keycloak.sh b/server/pbenchinacan/load_keycloak.sh index 16c284ea16..a6d273e4c0 100755 --- a/server/pbenchinacan/load_keycloak.sh +++ b/server/pbenchinacan/load_keycloak.sh @@ -67,9 +67,9 @@ fi # Create a client scope with custom mapper that a functional test # user can include in the OIDC token request over a REST call. -# This will instruct Keycloak to include the CLIENT_ID (pbench-dashboard) -# when someone request a token Over a rest API using a CLIENT_ID. -# Having CLIENT_ID in the aud claim of the token is essential for the token +# This will instruct Keycloak to include the (pbench-dashboard) +# when someone request a token Over a rest API using a . +# Having in the aud claim of the token is essential for the token # to be validated. curl -si -f -X POST "${KEYCLOAK_HOST_PORT}/admin/realms/${REALM}/client-scopes" \ -H "Authorization: Bearer ${ADMIN_TOKEN}" \ @@ -91,7 +91,7 @@ curl -si -f -X POST "${KEYCLOAK_HOST_PORT}/admin/realms/${REALM}/client-scopes" "protocolMapper": "oidc-audience-mapper", "consentRequired": false, "config": { - "included.client.audience": "pbench-dashboard", + "included.client.audience": "'${CLIENT_ID}'", "id.token.claim": "false", "access.token.claim": "true" } From 03f88d7ed9863bb9061c84297961c98e9217619e Mon Sep 17 00:00:00 2001 From: npalaska Date: Mon, 13 Feb 2023 13:56:13 -0500 Subject: [PATCH 4/9] fix the client name in the scope --- server/pbenchinacan/load_keycloak.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/pbenchinacan/load_keycloak.sh b/server/pbenchinacan/load_keycloak.sh index a6d273e4c0..9c81f613d5 100755 --- a/server/pbenchinacan/load_keycloak.sh +++ b/server/pbenchinacan/load_keycloak.sh @@ -91,7 +91,7 @@ curl -si -f -X POST "${KEYCLOAK_HOST_PORT}/admin/realms/${REALM}/client-scopes" "protocolMapper": "oidc-audience-mapper", "consentRequired": false, "config": { - "included.client.audience": "'${CLIENT_ID}'", + "included.client.audience": "'${CLIENT}'", "id.token.claim": "false", "access.token.claim": "true" } From d4d8788956f41e9a675acd92bf1738e8341aacf7 Mon Sep 17 00:00:00 2001 From: npalaska Date: Mon, 13 Feb 2023 14:51:31 -0500 Subject: [PATCH 5/9] fix the comment --- server/pbenchinacan/load_keycloak.sh | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/server/pbenchinacan/load_keycloak.sh b/server/pbenchinacan/load_keycloak.sh index 9c81f613d5..1f999720be 100755 --- a/server/pbenchinacan/load_keycloak.sh +++ b/server/pbenchinacan/load_keycloak.sh @@ -65,10 +65,9 @@ else echo "Created ${REALM} realm" fi -# Create a client scope with custom mapper that a functional test -# user can include in the OIDC token request over a REST call. -# This will instruct Keycloak to include the (pbench-dashboard) -# when someone request a token Over a rest API using a . +# Create a client scope with custom mapper that will instruct Keycloak +# to include the (pbench-dashboard) when someone request +# a token from Keycloak using a . # Having in the aud claim of the token is essential for the token # to be validated. curl -si -f -X POST "${KEYCLOAK_HOST_PORT}/admin/realms/${REALM}/client-scopes" \ From 2a94b6f7127acfc97dcc9b1f5506f6e3d626cd26 Mon Sep 17 00:00:00 2001 From: npalaska Date: Tue, 14 Feb 2023 16:01:39 -0500 Subject: [PATCH 6/9] refactor server client and oidc_admin --- lib/pbench/client/__init__.py | 49 ++++++++++--------- lib/pbench/client/oidc_admin.py | 5 +- lib/pbench/test/functional/server/conftest.py | 38 ++++++-------- server/pbenchinacan/load_keycloak.sh | 8 +-- 4 files changed, 48 insertions(+), 52 deletions(-) diff --git a/lib/pbench/client/__init__.py b/lib/pbench/client/__init__.py index 7d4ca3c82d..b40b8ef4dd 100644 --- a/lib/pbench/client/__init__.py +++ b/lib/pbench/client/__init__.py @@ -8,7 +8,8 @@ import requests from requests.structures import CaseInsensitiveDict -from pbench.client.types import Dataset, JSONMap, JSONOBJECT +from pbench.client.oidc_admin import OIDCAdmin +from pbench.client.types import Dataset, JSONOBJECT class PbenchClientError(Exception): @@ -86,6 +87,7 @@ def __init__(self, host: str): self.auth_token: Optional[str] = None self.session: Optional[requests.Session] = None self.endpoints: Optional[JSONOBJECT] = None + self.oidc_admin: Optional[OIDCAdmin] = None def _headers( self, user_headers: Optional[dict[str, str]] = None @@ -302,8 +304,13 @@ def delete( return response def connect(self, headers: Optional[dict[str, str]] = None) -> None: - """Connect to the Pbench Server host using the endpoints API to be sure - that it responds, and cache the endpoints response payload. + """Performs some pre-requisite actions to make server client usable. + + 1. Connect to the Pbench Server host using the endpoints API to be + sure that it responds, and cache the endpoints response payload. + + 2. Creates an OIDCAdmin object that server client can use to + perform admin privileged actions on OIDC server. This also allows the client to add default HTTP headers to the session which will be used for all operations unless overridden for specific @@ -321,31 +328,27 @@ def connect(self, headers: Optional[dict[str, str]] = None) -> None: self.endpoints = response.json() assert self.endpoints - def login(self, user: str, password: str) -> JSONMap: - """Login to a specified username with the password, and store the - resulting authentication token. + # Create an OIDCAdmin object and confirm the connection was successful + self.oidc_admin = OIDCAdmin(server_url=self.endpoints["openid"]["server"]) + assert self.oidc_admin.get_admin_token()["access_token"] + + def login(self, user: str, password: str): + """Login to a specified username with the password on OIDC server, + and store the resulting authentication token. Args: user: Account username password: Account password - - Returns: - The login response - """ - response = self.post(API.LOGIN, json={"username": user, "password": password}) - response.raise_for_status() - json = response.json() - self.username = json["username"] - self.auth_token = json["auth_token"] - return JSONMap(json) - - def logout(self) -> None: - """Logout the currently authenticated user and remove the - authentication token. """ - self.post(API.LOGOUT) - self.username = None - self.auth_token = None + response = self.oidc_admin.user_login( + client_id=self.endpoints["openid"]["client"], + username=user, + password=password, + ) + assert response["access_token"] + auth_token = response["access_token"] + self.username = user + self.auth_token = auth_token def upload(self, tarball: Path, **kwargs) -> requests.Response: """Upload a tarball to the server. diff --git a/lib/pbench/client/oidc_admin.py b/lib/pbench/client/oidc_admin.py index 84e68b5004..23ce4761de 100644 --- a/lib/pbench/client/oidc_admin.py +++ b/lib/pbench/client/oidc_admin.py @@ -8,7 +8,7 @@ class OIDCAdmin(Connection): - OIDC_REALM = "pbench-server" + OIDC_REALM = os.getenv("OIDC_REALM", "pbench-server") ADMIN_USERNAME = os.getenv("ADMIN_USERNAME", "admin") ADMIN_PASSWORD = os.getenv("ADMIN_PASSWORD", "admin") @@ -111,8 +111,7 @@ def user_login(self, client_id: str, username: str, password: str) -> dict: "username": username, "password": password, } - response = self.post(path=url_path, data=data, headers=headers) - return response.json() + return self.post(path=url_path, data=data, headers=headers).json() def get_user(self, username: str, token: str) -> dict: """Get the OIDC user representation dict. diff --git a/lib/pbench/test/functional/server/conftest.py b/lib/pbench/test/functional/server/conftest.py index 848bf661c8..0a61d83757 100644 --- a/lib/pbench/test/functional/server/conftest.py +++ b/lib/pbench/test/functional/server/conftest.py @@ -5,7 +5,12 @@ from pbench.client import PbenchServerClient from pbench.client.oidc_admin import OIDCAdmin -from pbench.client.types import JSONMap + +USERNAME: str = "tester" +EMAIL: str = "tester@gmail.com" +PASSWORD: str = "123456" +FIRST_NAME: str = "Test" +LAST_NAME: str = "User" @pytest.fixture(scope="module") @@ -32,20 +37,18 @@ def oidc_admin(server_client: PbenchServerClient): Used by Pbench Server functional tests to get admin access on OIDC server. """ - oidc_endpoints = server_client.endpoints["openid"] - oidc_server = OIDCAdmin(server_url=oidc_endpoints["server"]) - return oidc_server + return OIDCAdmin(server_url=server_client.endpoints["openid"]["server"]) @pytest.fixture(scope="module") def register_test_user(oidc_admin: OIDCAdmin): """Create a test user for functional tests.""" response = oidc_admin.create_new_user( - username="tester", - email="tester@gmail.com", - password="123456", - first_name="Test", - last_name="User", + username=USERNAME, + email=EMAIL, + password=PASSWORD, + first_name=FIRST_NAME, + last_name=LAST_NAME, ) # To allow testing outside our transient CI containers, allow the tester @@ -56,17 +59,8 @@ def register_test_user(oidc_admin: OIDCAdmin): @pytest.fixture -def login_user( - server_client: PbenchServerClient, oidc_admin: OIDCAdmin, register_test_user -): +def login_user(server_client: PbenchServerClient, register_test_user): """Log in the test user and return the authentication token""" - oidc_endpoints = server_client.endpoints["openid"] - response = oidc_admin.user_login( - client_id=oidc_endpoints["client"], username="tester", password="123456" - ) - auth_token = response["access_token"] - assert auth_token - json = {"username": "tester", "auth_token": auth_token} - server_client.username = "tester" - server_client.auth_token = auth_token - yield JSONMap(json) + server_client.login(USERNAME, PASSWORD) + assert server_client.auth_token + yield diff --git a/server/pbenchinacan/load_keycloak.sh b/server/pbenchinacan/load_keycloak.sh index 1f999720be..9fbab02ee0 100755 --- a/server/pbenchinacan/load_keycloak.sh +++ b/server/pbenchinacan/load_keycloak.sh @@ -23,8 +23,8 @@ ADMIN_USERNAME=${ADMIN_USERNAME:-"admin"} ADMIN_PASSWORD=${ADMIN_PASSWORD:-"admin"} # These values must match the options "realm" and "client in the # "openid-connect" section of the pbench server configuration file. -REALM=${KEYCLOAK_REALM:-"pbench-server"} -CLIENT=${KEYCLOAK_CLIENT:-"pbench-dashboard"} +REALM=${KEYCLOAK_REALM:-"pbench-server-test"} +CLIENT=${KEYCLOAK_CLIENT:-"pbench-dashboard-test"} end_in_epoch_secs=$(date --date "2 minutes" +%s) @@ -66,7 +66,7 @@ else fi # Create a client scope with custom mapper that will instruct Keycloak -# to include the (pbench-dashboard) when someone request +# to include the (pbench-dashboard) when someone requests # a token from Keycloak using a . # Having in the aud claim of the token is essential for the token # to be validated. @@ -158,7 +158,7 @@ else echo "Assigned an 'ADMIN' client role to the user 'admin' created above" fi -# Verify that the user id has a role 'ADMIN' assigned to it +# Verify that the user id has an 'ADMIN' role assigned to it USER_ROLES=$(curl -s "${KEYCLOAK_HOST_PORT}/admin/realms/${REALM}/users/${USER_ID}/role-mappings/clients/${CLIENT_ID}" \ -H "Authorization: Bearer ${ADMIN_TOKEN}" \ -H "Content-Type: application/json" | jq -r '.[].name') From c0c8625c787a3fb76c9eeb20e16da098f671c27d Mon Sep 17 00:00:00 2001 From: npalaska Date: Tue, 14 Feb 2023 19:00:00 -0500 Subject: [PATCH 7/9] fix client unit tests --- lib/pbench/client/__init__.py | 28 ++++++++++--------- lib/pbench/client/oidc_admin.py | 7 ++--- lib/pbench/server/auth/__init__.py | 15 ++++++---- lib/pbench/test/functional/server/conftest.py | 1 - .../test/functional/server/test_connect.py | 4 +-- lib/pbench/test/unit/client/conftest.py | 14 +++++++++- lib/pbench/test/unit/client/test_connect.py | 9 +++++- lib/pbench/test/unit/client/test_login.py | 21 ++++++-------- lib/pbench/test/unit/server/auth/test_auth.py | 11 ++++++-- server/pbenchinacan/load_keycloak.sh | 4 +-- 10 files changed, 71 insertions(+), 43 deletions(-) diff --git a/lib/pbench/client/__init__.py b/lib/pbench/client/__init__.py index b40b8ef4dd..5a6e9c146a 100644 --- a/lib/pbench/client/__init__.py +++ b/lib/pbench/client/__init__.py @@ -10,6 +10,7 @@ from pbench.client.oidc_admin import OIDCAdmin from pbench.client.types import Dataset, JSONOBJECT +from pbench.server.auth import OpenIDClientError class PbenchClientError(Exception): @@ -309,8 +310,8 @@ def connect(self, headers: Optional[dict[str, str]] = None) -> None: 1. Connect to the Pbench Server host using the endpoints API to be sure that it responds, and cache the endpoints response payload. - 2. Creates an OIDCAdmin object that server client can use to - perform admin privileged actions on OIDC server. + 2. Create an OIDCAdmin object that a server client can use to + perform privileged actions on an OIDC server. This also allows the client to add default HTTP headers to the session which will be used for all operations unless overridden for specific @@ -330,25 +331,26 @@ def connect(self, headers: Optional[dict[str, str]] = None) -> None: # Create an OIDCAdmin object and confirm the connection was successful self.oidc_admin = OIDCAdmin(server_url=self.endpoints["openid"]["server"]) - assert self.oidc_admin.get_admin_token()["access_token"] def login(self, user: str, password: str): - """Login to a specified username with the password on OIDC server, + """Log into the OIDC server using the specified username and password, and store the resulting authentication token. Args: user: Account username password: Account password """ - response = self.oidc_admin.user_login( - client_id=self.endpoints["openid"]["client"], - username=user, - password=password, - ) - assert response["access_token"] - auth_token = response["access_token"] - self.username = user - self.auth_token = auth_token + try: + response = self.oidc_admin.user_login( + client_id=self.endpoints["openid"]["client"], + username=user, + password=password, + ) + except OpenIDClientError: + self.auth_token = None + else: + self.username = user + self.auth_token = response["access_token"] def upload(self, tarball: Path, **kwargs) -> requests.Response: """Upload a tarball to the server. diff --git a/lib/pbench/client/oidc_admin.py b/lib/pbench/client/oidc_admin.py index 23ce4761de..a1064ddc15 100644 --- a/lib/pbench/client/oidc_admin.py +++ b/lib/pbench/client/oidc_admin.py @@ -1,5 +1,4 @@ from http import HTTPStatus -import json import os import requests @@ -9,8 +8,8 @@ class OIDCAdmin(Connection): OIDC_REALM = os.getenv("OIDC_REALM", "pbench-server") - ADMIN_USERNAME = os.getenv("ADMIN_USERNAME", "admin") - ADMIN_PASSWORD = os.getenv("ADMIN_PASSWORD", "admin") + ADMIN_USERNAME = os.getenv("OIDC_ADMIN_USERNAME", "admin") + ADMIN_PASSWORD = os.getenv("OIDC_ADMIN_PASSWORD", "admin") def __init__(self, server_url: str): super().__init__(server_url, verify=False) @@ -79,7 +78,7 @@ def create_new_user( {"type": "password", "value": password, "temporary": False} ], } - response = self.post(path=url_path, data=json.dumps(data), headers=headers) + response = self.post(path=url_path, json=data, headers=headers) return response def user_login(self, client_id: str, username: str, password: str) -> dict: diff --git a/lib/pbench/server/auth/__init__.py b/lib/pbench/server/auth/__init__.py index d3b5207b37..8a35bf5d2c 100644 --- a/lib/pbench/server/auth/__init__.py +++ b/lib/pbench/server/auth/__init__.py @@ -51,6 +51,7 @@ def _method( method: str, path: str, data: Union[Dict, str, None], + json: Optional[Dict] = None, headers: Optional[Dict] = None, **kwargs, ) -> requests.Response: @@ -59,7 +60,8 @@ def _method( Args: method : The API HTTP method path : Path for the request. - data : Json data to send with the request in case of the POST + data : Data to send with the request in case of the POST + json: JSON data to send with the request in case of the POST kwargs : Additional keyword args Returns: @@ -72,6 +74,7 @@ def _method( kwargs = dict( params=kwargs, data=data, + json=json, headers=final_headers, verify=self.verify, ) @@ -123,12 +126,13 @@ def get( Returns: Response from the request. """ - return self._method("GET", path, None, headers=headers, **kwargs) + return self._method("GET", path, None, None, headers=headers, **kwargs) def post( self, path: str, - data: Union[Dict, str], + data: Union[Dict, str] = None, + json: Optional[Dict] = None, headers: Optional[Dict] = None, **kwargs, ) -> requests.Response: @@ -137,14 +141,15 @@ def post( Args: path : Path for the request - data : JSON request body + data : Request body to attach + json: JSON request body headers : Additional headers to add to the request kwargs : Additional keyword args to be added as URL parameters Returns: Response from the request. """ - return self._method("POST", path, data, headers=headers, **kwargs) + return self._method("POST", path, data, json, headers=headers, **kwargs) @dataclass diff --git a/lib/pbench/test/functional/server/conftest.py b/lib/pbench/test/functional/server/conftest.py index 0a61d83757..060f7b257b 100644 --- a/lib/pbench/test/functional/server/conftest.py +++ b/lib/pbench/test/functional/server/conftest.py @@ -63,4 +63,3 @@ def login_user(server_client: PbenchServerClient, register_test_user): """Log in the test user and return the authentication token""" server_client.login(USERNAME, PASSWORD) assert server_client.auth_token - yield diff --git a/lib/pbench/test/functional/server/test_connect.py b/lib/pbench/test/functional/server/test_connect.py index d29a2f0886..f58857fe8f 100644 --- a/lib/pbench/test/functional/server/test_connect.py +++ b/lib/pbench/test/functional/server/test_connect.py @@ -29,5 +29,5 @@ def test_connect(self, server_client: PbenchServerClient): # verify all the required openid-connect fields are present if "openid" in endpoints: - expected = {"server", "client", "realm", "secret"} - assert set(endpoints["openid"]) >= expected + expected = {"server", "client", "realm"} + assert set(endpoints["openid"]) == expected diff --git a/lib/pbench/test/unit/client/conftest.py b/lib/pbench/test/unit/client/conftest.py index 9496cd9302..a3e87f3d21 100644 --- a/lib/pbench/test/unit/client/conftest.py +++ b/lib/pbench/test/unit/client/conftest.py @@ -16,8 +16,20 @@ def connect(): f"{pbench.url}/api/v1/endpoints", json={ "identification": "string", - "api": {"login": f"{pbench.url}/api/v1/login"}, + "api": {}, "uri": {}, + "openid": { + "server": "http://oidc_server", + "realm": "pbench-server", + "client": "pbench-client", + }, + }, + ) + responses.add( + responses.POST, + "http://oidc_server/realms/master/protocol/openid-connect/token", + json={ + "access_token": "admin_token", }, ) pbench.connect({"accept": "application/json"}) diff --git a/lib/pbench/test/unit/client/test_connect.py b/lib/pbench/test/unit/client/test_connect.py index 36e018deb5..0b9ef4d09a 100644 --- a/lib/pbench/test/unit/client/test_connect.py +++ b/lib/pbench/test/unit/client/test_connect.py @@ -22,12 +22,18 @@ def test_construct_url(self): def test_connect(self): pbench = PbenchServerClient("10.1.100.2") url = f"{pbench.url}/api/v1/endpoints" + openid_dict = {"server": "http://oidc_server", "client": "pbench_client"} with responses.RequestsMock() as rsp: rsp.add( responses.GET, url, - json={"identification": "string", "api": {}, "uri": {}}, + json={ + "identification": "string", + "api": {}, + "uri": {}, + "openid": openid_dict, + }, ) pbench.connect({"accept": "application/json"}) assert len(rsp.calls) == 1 @@ -51,3 +57,4 @@ def test_connect(self): assert endpoints["api"] == {} assert endpoints["identification"] == "string" assert endpoints["uri"] == {} + assert endpoints["openid"] == openid_dict diff --git a/lib/pbench/test/unit/client/test_login.py b/lib/pbench/test/unit/client/test_login.py index 6f23192845..94747439e7 100644 --- a/lib/pbench/test/unit/client/test_login.py +++ b/lib/pbench/test/unit/client/test_login.py @@ -1,7 +1,5 @@ from http import HTTPStatus -import pytest -import requests import responses @@ -11,11 +9,11 @@ def test_login(self, connect): Confirm that a successful Pbench Server login captures the 'username' and 'auth_token' in the client object. """ - url = f"{connect.url}/api/v1/login" + oidc_server = connect.endpoints["openid"]["server"] + oidc_realm = connect.endpoints["openid"]["realm"] + url = f"{oidc_server}/realms/{oidc_realm}/protocol/openid-connect/token" with responses.RequestsMock() as rsp: - rsp.add( - responses.POST, url, json={"username": "user", "auth_token": "foobar"} - ) + rsp.add(responses.POST, url, json={"access_token": "foobar"}) connect.login("user", "password") assert len(rsp.calls) == 1 assert rsp.calls[0].request.url == url @@ -30,18 +28,17 @@ def test_bad_login(self, connect): handled by the client library, and does not set the client 'username` and 'auth_token' properties. """ - url = f"{connect.url}/api/v1/login" + oidc_server = connect.endpoints["openid"]["server"] + oidc_realm = connect.endpoints["openid"]["realm"] + url = f"{oidc_server}/realms/{oidc_realm}/protocol/openid-connect/token" with responses.RequestsMock() as rsp: rsp.add( responses.POST, url, status=HTTPStatus.UNAUTHORIZED, - json={"username": "user", "auth_token": "foobar"}, + json={"error_description": "Invalid user credentials"}, ) - - with pytest.raises(requests.HTTPError): - connect.login("user", "password") - + connect.login("user", "password") assert len(rsp.calls) == 1 assert rsp.calls[0].request.url == url assert rsp.calls[0].response.status_code == 401 diff --git a/lib/pbench/test/unit/server/auth/test_auth.py b/lib/pbench/test/unit/server/auth/test_auth.py index 0a97b990dc..b88af85aad 100644 --- a/lib/pbench/test/unit/server/auth/test_auth.py +++ b/lib/pbench/test/unit/server/auth/test_auth.py @@ -43,11 +43,12 @@ def fake_method(self, monkeypatch): args = {} def fake_method( - the_self, method: str, path: str, data: Dict, **kwargs + the_self, method: str, path: str, data: Dict, json: Dict, **kwargs ) -> requests.Response: args["method"] = method args["path"] = path args["data"] = data + args["json"] = json args["kwargs"] = kwargs return requests.Response() @@ -114,6 +115,7 @@ def request(self, *args, **kwargs): "HEAD", "/this/that", None, + None, headers={"header1": "one"}, this="that", that="this", @@ -123,6 +125,7 @@ def request(self, *args, **kwargs): assert response.args[1] == "https://example.com/this/that" assert response.kwargs["params"] == {"this": "that", "that": "this"} assert response.kwargs["data"] is None + assert response.kwargs["json"] is None assert response.kwargs["headers"] == {"header0": "zero", "header1": "one"} assert response.kwargs["verify"] is False @@ -160,6 +163,7 @@ def test_get(self, fake_method, conn): assert args["method"] == "GET" assert args["path"] == "foo/bar" assert args["data"] is None + assert args["json"] is None assert args["kwargs"] == {"headers": None, "this": "that", "then": "now"} def test_post(self, fake_method, conn): @@ -171,10 +175,13 @@ def test_post(self, fake_method, conn): conn : an existing Connection object to use for testing """ args = fake_method - response = conn.post("foo/bar", {"one": "two", "three": "four"}, five="six") + response = conn.post( + "foo/bar", {"one": "two", "three": "four"}, None, five="six" + ) assert response is not None assert args["method"] == "POST" assert args["path"] == "foo/bar" + assert args["json"] is None assert args["data"] == {"one": "two", "three": "four"} assert args["kwargs"] == {"headers": None, "five": "six"} diff --git a/server/pbenchinacan/load_keycloak.sh b/server/pbenchinacan/load_keycloak.sh index 9fbab02ee0..7c23c5a186 100755 --- a/server/pbenchinacan/load_keycloak.sh +++ b/server/pbenchinacan/load_keycloak.sh @@ -23,8 +23,8 @@ ADMIN_USERNAME=${ADMIN_USERNAME:-"admin"} ADMIN_PASSWORD=${ADMIN_PASSWORD:-"admin"} # These values must match the options "realm" and "client in the # "openid-connect" section of the pbench server configuration file. -REALM=${KEYCLOAK_REALM:-"pbench-server-test"} -CLIENT=${KEYCLOAK_CLIENT:-"pbench-dashboard-test"} +REALM=${KEYCLOAK_REALM:-"pbench-server"} +CLIENT=${KEYCLOAK_CLIENT:-"pbench-dashboard"} end_in_epoch_secs=$(date --date "2 minutes" +%s) From 9f70672f74eccdd4b73a6f9f9c1f8970a3167327 Mon Sep 17 00:00:00 2001 From: npalaska Date: Wed, 15 Feb 2023 16:23:29 -0500 Subject: [PATCH 8/9] review feedback --- lib/pbench/client/__init__.py | 19 ++++------- lib/pbench/client/oidc_admin.py | 5 +-- lib/pbench/server/auth/__init__.py | 32 +++++++++---------- .../test/functional/server/test_connect.py | 4 +-- lib/pbench/test/unit/client/test_login.py | 6 +++- lib/pbench/test/unit/server/auth/test_auth.py | 11 +++++-- 6 files changed, 39 insertions(+), 38 deletions(-) diff --git a/lib/pbench/client/__init__.py b/lib/pbench/client/__init__.py index 5a6e9c146a..85c6390179 100644 --- a/lib/pbench/client/__init__.py +++ b/lib/pbench/client/__init__.py @@ -10,7 +10,6 @@ from pbench.client.oidc_admin import OIDCAdmin from pbench.client.types import Dataset, JSONOBJECT -from pbench.server.auth import OpenIDClientError class PbenchClientError(Exception): @@ -340,17 +339,13 @@ def login(self, user: str, password: str): user: Account username password: Account password """ - try: - response = self.oidc_admin.user_login( - client_id=self.endpoints["openid"]["client"], - username=user, - password=password, - ) - except OpenIDClientError: - self.auth_token = None - else: - self.username = user - self.auth_token = response["access_token"] + response = self.oidc_admin.user_login( + client_id=self.endpoints["openid"]["client"], + username=user, + password=password, + ) + self.username = user + self.auth_token = response["access_token"] def upload(self, tarball: Path, **kwargs) -> requests.Response: """Upload a tarball to the server. diff --git a/lib/pbench/client/oidc_admin.py b/lib/pbench/client/oidc_admin.py index a1064ddc15..99d4952185 100644 --- a/lib/pbench/client/oidc_admin.py +++ b/lib/pbench/client/oidc_admin.py @@ -64,7 +64,6 @@ def create_new_user( admin_token = self.get_admin_token().get("access_token") url_path = f"/admin/realms/{self.OIDC_REALM}/users" headers = { - "Content-Type": "application/json", "Authorization": f"Bearer {admin_token}", } data = { @@ -102,7 +101,6 @@ def user_login(self, client_id: str, username: str, password: str) -> dict: """ url_path = f"/realms/{self.OIDC_REALM}/protocol/openid-connect/token" - headers = {"Content-Type": "application/x-www-form-urlencoded"} data = { "client_id": client_id, "grant_type": "password", @@ -110,7 +108,7 @@ def user_login(self, client_id: str, username: str, password: str) -> dict: "username": username, "password": password, } - return self.post(path=url_path, data=data, headers=headers).json() + return self.post(path=url_path, data=data).json() def get_user(self, username: str, token: str) -> dict: """Get the OIDC user representation dict. @@ -138,7 +136,6 @@ def get_user(self, username: str, token: str) -> dict: response = self.get( f"admin/realms/{self.OIDC_REALM}/users", headers={ - "Content-Type": "application/json", "Authorization": f"Bearer {token}", }, username=username, diff --git a/lib/pbench/server/auth/__init__.py b/lib/pbench/server/auth/__init__.py index 8a35bf5d2c..1fdc2f7e2b 100644 --- a/lib/pbench/server/auth/__init__.py +++ b/lib/pbench/server/auth/__init__.py @@ -4,7 +4,7 @@ from dataclasses import dataclass from http import HTTPStatus import logging -from typing import Dict, Optional, Union +from typing import Any, Dict, Optional from urllib.parse import urljoin import jwt @@ -38,7 +38,7 @@ class Connection: def __init__( self, server_url: str, - headers: Optional[Dict[str, str]] = None, + headers: Optional[dict[str, str]] = None, verify: bool = True, ): self.server_url = server_url @@ -50,9 +50,9 @@ def _method( self, method: str, path: str, - data: Union[Dict, str, None], - json: Optional[Dict] = None, - headers: Optional[Dict] = None, + data: Optional[Any] = None, + json: Optional[dict[str, Any]] = None, + headers: Optional[dict[str, str]] = None, **kwargs, ) -> requests.Response: """Common frontend for the HTTP operations on OIDC client connection. @@ -60,8 +60,8 @@ def _method( Args: method : The API HTTP method path : Path for the request. - data : Data to send with the request in case of the POST - json: JSON data to send with the request in case of the POST + data : Form data to send with the request in case of the POST + json : JSON data to send with the request in case of the POST kwargs : Additional keyword args Returns: @@ -71,7 +71,7 @@ def _method( if headers is not None: final_headers.update(headers) url = urljoin(self.server_url, path) - kwargs = dict( + request_dict = dict( params=kwargs, data=data, json=json, @@ -79,7 +79,7 @@ def _method( verify=self.verify, ) try: - response = self._connection.request(method, url, **kwargs) + response = self._connection.request(method, url, **request_dict) except requests.exceptions.ConnectionError as exc: raise OpenIDClientError( http_status=HTTPStatus.BAD_GATEWAY, @@ -113,7 +113,7 @@ def _method( return response def get( - self, path: str, headers: Optional[Dict] = None, **kwargs + self, path: str, headers: Optional[dict[str, str]] = None, **kwargs ) -> requests.Response: """GET wrapper to handle an authenticated GET operation on the Resource at a given path. @@ -126,14 +126,14 @@ def get( Returns: Response from the request. """ - return self._method("GET", path, None, None, headers=headers, **kwargs) + return self._method("GET", path, headers=headers, **kwargs) def post( self, path: str, - data: Union[Dict, str] = None, - json: Optional[Dict] = None, - headers: Optional[Dict] = None, + data: Optional[Any] = None, + json: Optional[dict[str, Any]] = None, + headers: Optional[dict[str, str]] = None, **kwargs, ) -> requests.Response: """POST wrapper to handle an authenticated POST operation on the @@ -141,8 +141,8 @@ def post( Args: path : Path for the request - data : Request body to attach - json: JSON request body + data : Optional HTML form body to attach + json : JSON request body headers : Additional headers to add to the request kwargs : Additional keyword args to be added as URL parameters diff --git a/lib/pbench/test/functional/server/test_connect.py b/lib/pbench/test/functional/server/test_connect.py index f58857fe8f..b0f0664121 100644 --- a/lib/pbench/test/functional/server/test_connect.py +++ b/lib/pbench/test/functional/server/test_connect.py @@ -29,5 +29,5 @@ def test_connect(self, server_client: PbenchServerClient): # verify all the required openid-connect fields are present if "openid" in endpoints: - expected = {"server", "client", "realm"} - assert set(endpoints["openid"]) == expected + expected = {"client", "realm", "server"} + assert set(endpoints["openid"]) >= expected diff --git a/lib/pbench/test/unit/client/test_login.py b/lib/pbench/test/unit/client/test_login.py index 94747439e7..650b52d9a9 100644 --- a/lib/pbench/test/unit/client/test_login.py +++ b/lib/pbench/test/unit/client/test_login.py @@ -1,7 +1,10 @@ from http import HTTPStatus +import pytest import responses +from pbench.server.auth import OpenIDClientError + class TestLogin: def test_login(self, connect): @@ -38,7 +41,8 @@ def test_bad_login(self, connect): status=HTTPStatus.UNAUTHORIZED, json={"error_description": "Invalid user credentials"}, ) - connect.login("user", "password") + with pytest.raises(OpenIDClientError): + connect.login("user", "password") assert len(rsp.calls) == 1 assert rsp.calls[0].request.url == url assert rsp.calls[0].response.status_code == 401 diff --git a/lib/pbench/test/unit/server/auth/test_auth.py b/lib/pbench/test/unit/server/auth/test_auth.py index b88af85aad..c3343d8da2 100644 --- a/lib/pbench/test/unit/server/auth/test_auth.py +++ b/lib/pbench/test/unit/server/auth/test_auth.py @@ -1,7 +1,7 @@ import configparser from dataclasses import dataclass from http import HTTPStatus -from typing import Dict, Optional, Tuple, Union +from typing import Any, Dict, Optional, Tuple, Union from flask import current_app, Flask import jwt @@ -43,7 +43,12 @@ def fake_method(self, monkeypatch): args = {} def fake_method( - the_self, method: str, path: str, data: Dict, json: Dict, **kwargs + the_self, + method: str, + path: str, + data: Optional[Any] = None, + json: Optional[dict[str, Any]] = None, + **kwargs, ) -> requests.Response: args["method"] = method args["path"] = path @@ -176,7 +181,7 @@ def test_post(self, fake_method, conn): """ args = fake_method response = conn.post( - "foo/bar", {"one": "two", "three": "four"}, None, five="six" + "foo/bar", data={"one": "two", "three": "four"}, five="six" ) assert response is not None assert args["method"] == "POST" From b725ee7a980597670b64c8f91228b6b7fa8af487 Mon Sep 17 00:00:00 2001 From: npalaska Date: Thu, 16 Feb 2023 10:09:31 -0500 Subject: [PATCH 9/9] remove the internal test_user functional tests --- lib/pbench/client/oidc_admin.py | 8 +- lib/pbench/server/auth/__init__.py | 4 +- .../test/functional/server/test_user.py | 214 ------------------ 3 files changed, 4 insertions(+), 222 deletions(-) delete mode 100644 lib/pbench/test/functional/server/test_user.py diff --git a/lib/pbench/client/oidc_admin.py b/lib/pbench/client/oidc_admin.py index 99d4952185..d21a6cb0d9 100644 --- a/lib/pbench/client/oidc_admin.py +++ b/lib/pbench/client/oidc_admin.py @@ -63,9 +63,7 @@ def create_new_user( """ admin_token = self.get_admin_token().get("access_token") url_path = f"/admin/realms/{self.OIDC_REALM}/users" - headers = { - "Authorization": f"Bearer {admin_token}", - } + headers = {"Authorization": f"Bearer {admin_token}"} data = { "username": username, "email": email, @@ -135,9 +133,7 @@ def get_user(self, username: str, token: str) -> dict: """ response = self.get( f"admin/realms/{self.OIDC_REALM}/users", - headers={ - "Authorization": f"Bearer {token}", - }, + headers={"Authorization": f"Bearer {token}"}, username=username, ) if response.status_code == HTTPStatus.OK: diff --git a/lib/pbench/server/auth/__init__.py b/lib/pbench/server/auth/__init__.py index 1fdc2f7e2b..9dd1b1806d 100644 --- a/lib/pbench/server/auth/__init__.py +++ b/lib/pbench/server/auth/__init__.py @@ -4,7 +4,7 @@ from dataclasses import dataclass from http import HTTPStatus import logging -from typing import Any, Dict, Optional +from typing import Any, Optional from urllib.parse import urljoin import jwt @@ -332,7 +332,7 @@ def __init__( client_id: str, realm_name: str, verify: bool = True, - headers: Optional[Dict[str, str]] = None, + headers: Optional[dict[str, str]] = None, ): """Initialize an OpenID Connect Client object. diff --git a/lib/pbench/test/functional/server/test_user.py b/lib/pbench/test/functional/server/test_user.py deleted file mode 100644 index 5aef76a211..0000000000 --- a/lib/pbench/test/functional/server/test_user.py +++ /dev/null @@ -1,214 +0,0 @@ -# from http import HTTPStatus -# -# import pytest -# from requests import HTTPError -# -# from pbench.client import API, PbenchServerClient -# -# USERNAME1 = "functional" -# USERNAME2 = "nonfunctional" -# PASSWORD = "tests" -# -# -# class TestUser: -# """ -# Pbench Server functional tests for basic "user" operations. -# -# NOTE: The functional test system is intended to run on a pristine -# containerized Pbench Server. We could check for some pre-conditions if we -# had a known Administrative user (e.g., admin/admin). -# """ -# -# def test_login_fail(self, server_client: PbenchServerClient): -# """ -# Trying to log in to a non-existent user should fail. -# -# NOTE: This will fail if "nouser" exists. -# """ -# with pytest.raises( -# HTTPError, -# match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/login", -# ): -# server_client.login("nouser", "nopassword") -# -# @pytest.mark.parametrize( -# "json", -# [ -# { -# "username": USERNAME1, -# "first_name": "Test", -# "last_name": "Account", -# "password": PASSWORD, -# "email": f"{USERNAME1}@gmail.com", -# }, -# { -# "username": USERNAME2, -# "first_name": "Tester", -# "last_name": "Accountant", -# "password": PASSWORD, -# "email": f"{USERNAME2}@gmail.com", -# }, -# ], -# ) -# def test_register(self, server_client: PbenchServerClient, json): -# """ -# Test that we can register a new user. -# -# NOTE: This will fail if the username already exists. -# """ -# response = server_client.post(API.REGISTER, json=json, raise_error=False) -# assert ( -# response.status_code == HTTPStatus.CREATED -# ), f"Register failed with {response.json()}" -# -# def test_register_redux(self, server_client: PbenchServerClient): -# """ -# Test that an attempt to register an existing user fails. -# """ -# json = { -# "username": USERNAME1, -# "first_name": "Repeat", -# "last_name": "Redux", -# "password": PASSWORD, -# "email": f"{USERNAME1}@gmail.com", -# } -# with pytest.raises( -# HTTPError, -# match=f"FORBIDDEN for url: {server_client.scheme}://.*?/api/v1/register", -# ): -# server_client.post(API.REGISTER, json=json) -# -# def test_profile_noauth(self, server_client: PbenchServerClient): -# """ -# Test that we can't access a user profile without authentication. -# """ -# with pytest.raises( -# HTTPError, -# match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME1}", -# ): -# server_client.get(API.USER, {"target_username": USERNAME1}) -# -# def test_login(self, server_client: PbenchServerClient): -# """ -# Test that we can log in using our new user. -# -# NOTE: This assumes test cases will be run in order. There are Pytest -# plugins to explicitly control test case order, but Pytest generally -# does run tests within a single class in order. Is it worth taking on -# another dependency to make this explicit? -# """ -# server_client.login(USERNAME1, PASSWORD) -# assert server_client.auth_token -# -# def test_profile_bad_auth(self, server_client: PbenchServerClient): -# """ -# Test that we can't access a user profile with an invalid authentication -# token. -# """ -# with pytest.raises( -# HTTPError, -# match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME1}", -# ): -# server_client.get( -# API.USER, -# {"target_username": USERNAME1}, -# headers={"Authorization": "Bearer of bad tokens"}, -# ) -# -# def test_profile_not_self(self, server_client: PbenchServerClient): -# """ -# Test that we can't access another user's profile. -# """ -# with pytest.raises( -# HTTPError, -# match=f"FORBIDDEN for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME2}", -# ): -# server_client.get(API.USER, {"target_username": USERNAME2}) -# -# def test_profile(self, server_client: PbenchServerClient): -# """ -# Test that we can retrieve our own user profile once logged in. -# -# NOTE: This assumes test cases will be run in order. -# """ -# response = server_client.get(API.USER, {"target_username": USERNAME1}) -# assert response.json()["username"] == USERNAME1 -# -# def test_update(self, server_client: PbenchServerClient): -# """ -# Test that we can update our own user profile once logged in. -# -# NOTE: This assumes test cases will be run in order. -# """ -# response = server_client.get(API.USER, {"target_username": USERNAME1}) -# user = response.json() -# assert user["first_name"] != "Mycroft" -# user["first_name"] = "Mycroft" -# -# # Remove fields that PUT will reject, but use a copy so that we can -# # compare the new value against what we expect. -# # -# # This is unfortunate: PUT should IGNORE unmodifiable fields to allow a -# # standard REST GET/update/PUT sequence. -# payload = user.copy() -# del payload["registered_on"] -# -# response = server_client.put( -# API.USER, {"target_username": USERNAME1}, json=payload -# ) -# put_response = response.json() -# -# response = server_client.get(API.USER, {"target_username": USERNAME1}) -# updated = response.json() -# assert user == updated -# assert put_response == user -# -# def test_delete_other(self, server_client: PbenchServerClient): -# """ -# Test that we can't delete another user. -# -# NOTE: This assumes test cases will be run in order. -# """ -# with pytest.raises( -# HTTPError, -# match=f"FORBIDDEN for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME2}", -# ): -# server_client.delete(API.USER, {"target_username": USERNAME2}) -# -# def test_delete_nologin(self, server_client: PbenchServerClient): -# """ -# Test that we can't delete a user profile when not logged in. -# -# NOTE: This assumes test cases will be run in order. -# """ -# server_client.logout() -# with pytest.raises( -# HTTPError, -# match=f"UNAUTHORIZED for url: {server_client.scheme}://.*?/api/v1/user/{USERNAME1}", -# ): -# server_client.delete(API.USER, {"target_username": USERNAME1}) -# -# def test_delete(self, server_client: PbenchServerClient): -# """ -# Test that we can delete our own user profile once logged in. -# -# NOTE: This assumes test cases will be run in order. -# """ -# server_client.login(USERNAME1, PASSWORD) -# server_client.delete(API.USER, {"target_username": USERNAME1}) -# -# def test_logout(self, server_client: PbenchServerClient): -# """ -# Test that we can log out from our session. -# -# NOTE: This assumes test cases will be run in order. -# """ -# server_client.logout() -# assert server_client.auth_token is None -# -# def test_cleanup_user2(self, server_client: PbenchServerClient): -# """ -# Remove the second test user to make the test idempotent. -# """ -# server_client.login(USERNAME2, PASSWORD) -# server_client.delete(API.USER, {"target_username": USERNAME2})