From e25dba82093acb1a2f7a367363fe064cae77d3d5 Mon Sep 17 00:00:00 2001 From: Lukas Holecek Date: Thu, 15 Feb 2024 08:59:03 +0100 Subject: [PATCH] Fix authorization failure responses (#137) Messages and logging need to be improved for the new authenticated API to troubleshoot issues. Failure to authorize a request with the service must be logged. If permission is denied to post a data (or access an endpoint), 403 Forbidden status code must be returned (currently it is 401). Failure to find user in LDAP should be send back to user (with 403 response including the test case name) In case of LDAP errors, the service should return 502 status code (not 4XX). Return JSON formatted responses consistently. JIRA: RHELWF-10633 --- resultsdb/__init__.py | 20 +++++++ resultsdb/authorization.py | 26 ++++----- testing/functest_api_v20.py | 2 +- testing/test_api_v3.py | 106 +++++++++++++++++++++++++++++++++--- 4 files changed, 132 insertions(+), 22 deletions(-) diff --git a/resultsdb/__init__.py b/resultsdb/__init__.py index c28486d..42de611 100644 --- a/resultsdb/__init__.py +++ b/resultsdb/__init__.py @@ -200,10 +200,30 @@ def register_handlers(app): def bad_request(error): return jsonify({"message": "Bad request"}), 400 + @app.errorhandler(401) + def unauthorized(error): + app.logger.warning("Unauthorized access: %s", error) + return jsonify({"message": str(error)}), 401 + + @app.errorhandler(403) + def forbidden(error): + app.logger.warning("Permission denied: %s", error) + return jsonify({"message": str(error)}), 403 + @app.errorhandler(404) def not_found(error): return jsonify({"message": "Not found"}), 404 + @app.errorhandler(500) + def internal_server_error(error): + app.logger.error("Internal error: %s", error) + return jsonify({"message": "Internal Server Error"}), 500 + + @app.errorhandler(502) + def bad_gateway(error): + app.logger.error("External error received: %s", error) + return jsonify({"message": "Bad Gateway"}), 502 + def init_session(app): app.config["SESSION_SQLALCHEMY"] = db diff --git a/resultsdb/authorization.py b/resultsdb/authorization.py index 6411aa0..63235fa 100644 --- a/resultsdb/authorization.py +++ b/resultsdb/authorization.py @@ -5,7 +5,7 @@ from werkzeug.exceptions import ( BadGateway, InternalServerError, - Unauthorized, + Forbidden, ) log = logging.getLogger(__name__) @@ -24,11 +24,11 @@ def get_group_membership(ldap, user, con, ldap_search): log.exception("LDAP_SEARCHES parameter should contain the BASE key") raise InternalServerError("LDAP_SEARCHES parameter should contain the BASE key") except ldap.SERVER_DOWN: - log.exception("The LDAP server is not reachable.") - raise BadGateway("The LDAP server is not reachable.") + log.exception("The LDAP server is not reachable") + raise BadGateway("The LDAP server is not reachable") except ldap.LDAPError: - log.exception("Some error occurred initializing the LDAP connection.") - raise Unauthorized("Some error occurred initializing the LDAP connection.") + log.exception("Some error occurred initializing the LDAP connection") + raise BadGateway("Some error occurred initializing the LDAP connection") def match_testcase_permissions(testcase, permissions): @@ -44,7 +44,7 @@ def match_testcase_permissions(testcase, permissions): def verify_authorization(user, testcase, permissions, ldap_host, ldap_searches): if not (ldap_host and ldap_searches): raise InternalServerError( - "LDAP_HOST and LDAP_SEARCHES also need to be defined " "if PERMISSIONS is defined." + "LDAP_HOST and LDAP_SEARCHES also need to be defined if PERMISSIONS is defined" ) allowed_groups = [] @@ -56,13 +56,13 @@ def verify_authorization(user, testcase, permissions, ldap_host, ldap_searches): try: import ldap except ImportError: - raise InternalServerError("If PERMISSIONS is defined, python-ldap needs to be installed.") + raise InternalServerError("If PERMISSIONS is defined, python-ldap needs to be installed") try: con = ldap.initialize(ldap_host) except ldap.LDAPError: - log.exception("Some error occurred initializing the LDAP connection.") - raise Unauthorized("Some error occurred initializing the LDAP connection.") + log.exception("Some error occurred initializing the LDAP connection") + raise BadGateway("Some error occurred initializing the LDAP connection") any_groups_found = False for cur_ldap_search in ldap_searches: @@ -71,9 +71,7 @@ def verify_authorization(user, testcase, permissions, ldap_host, ldap_searches): return True any_groups_found = any_groups_found or len(groups) > 0 - if not any_groups_found: - raise Unauthorized(f"Failed to find user {user} in LDAP") - - raise Unauthorized( - f"User {user} is not authorized to submit a result for the test case {testcase}" + raise Forbidden( + f"User {user} is not authorized to submit results for the test case {testcase}" + + ("" if any_groups_found else "; failed to find the user in LDAP") ) diff --git a/testing/functest_api_v20.py b/testing/functest_api_v20.py index d6601e3..430b563 100644 --- a/testing/functest_api_v20.py +++ b/testing/functest_api_v20.py @@ -52,7 +52,7 @@ def require_postgres(self): "This test requires PostgreSQL to work properly. " "You can disable it by setting NO_CAN_HAS_POSTGRES " "env variable to any non-empty value.\n" - f'Current DB URI: { app.config["SQLALCHEMY_DATABASE_URI"] }' + f'Current DB URI: {app.config["SQLALCHEMY_DATABASE_URI"]}' ) assert db.engine.name == "postgresql" diff --git a/testing/test_api_v3.py b/testing/test_api_v3.py index c171c2c..57dcced 100644 --- a/testing/test_api_v3.py +++ b/testing/test_api_v3.py @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0+ from unittest.mock import ANY, patch, Mock +import ldap import pytest from resultsdb.models import db @@ -225,7 +226,7 @@ def test_api_v3_permissions_for_testcase_not_matching(client, permissions): assert r.json == [] -def test_api_v3_permission_denied(client, permissions): +def test_api_v3_permission_denied(client, permissions, caplog): permissions.append( { "users": ["testuser2"], @@ -234,10 +235,15 @@ def test_api_v3_permission_denied(client, permissions): ) data = brew_build_request_data() r = client.post("/api/v3/results/brew-builds", json=data) - assert r.status_code == 401, r.text - assert ( - "User testuser1 is not authorized to submit a result for the test case testcase1" in r.text + assert r.status_code == 403, r.text + expected_error = ( + "403 Forbidden: User testuser1 is not authorized to submit results" + " for the test case testcase1" ) + assert expected_error in r.text + assert {"message": ANY} == r.json + assert expected_error == r.json["message"] + assert f"Permission denied: {expected_error}" in caplog.text def test_api_v3_permission_matches_username(client, permissions): @@ -267,7 +273,86 @@ def test_api_v3_permission_matches_user_group(client, permissions, mock_ldap): ) -def test_api_v3_permission_no_groups_found(client, permissions, mock_ldap): +def test_api_v3_permission_ldap_server_down(client, permissions, mock_ldap, caplog): + permissions.append( + { + "groups": ["testgroup1"], + "testcases": ["testcase1*"], + } + ) + data = brew_build_request_data() + mock_ldap.search_s.side_effect = ldap.SERVER_DOWN() + r = client.post("/api/v3/results/brew-builds", json=data) + assert r.status_code == 502, r.text + mock_ldap.search_s.assert_called_once_with( + "ou=Groups,dc=example,dc=com", ANY, "(memberUid=testuser1)", ["cn"] + ) + assert {"message": "Bad Gateway"} == r.json + assert ( + "External error received: 502 Bad Gateway: The LDAP server is not reachable" + ) in caplog.text + + +def test_api_v3_permission_ldap_error(client, permissions, mock_ldap, caplog): + permissions.append( + { + "groups": ["testgroup1"], + "testcases": ["testcase1*"], + } + ) + data = brew_build_request_data() + mock_ldap.search_s.side_effect = ldap.LDAPError() + r = client.post("/api/v3/results/brew-builds", json=data) + assert r.status_code == 502, r.text + mock_ldap.search_s.assert_called_once_with( + "ou=Groups,dc=example,dc=com", ANY, "(memberUid=testuser1)", ["cn"] + ) + assert {"message": "Bad Gateway"} == r.json + assert ( + "External error received: 502 Bad Gateway:" + " Some error occurred initializing the LDAP connection" + ) in caplog.text + + +def test_api_v3_permission_ldap_misconfigured(client, permissions, mock_ldap, caplog, app): + permissions.append( + { + "groups": ["testgroup1"], + "testcases": ["testcase1*"], + } + ) + data = brew_build_request_data() + with patch.dict(app.config, {"LDAP_SEARCHES": [{}]}): + r = client.post("/api/v3/results/brew-builds", json=data) + assert r.status_code == 500, r.text + mock_ldap.search_s.assert_not_called() + assert {"message": "Internal Server Error"} == r.json + assert ( + "Internal error: 500 Internal Server Error:" + " LDAP_SEARCHES parameter should contain the BASE key" + ) in caplog.text + + +def test_api_v3_permission_ldap_not_configured(client, permissions, mock_ldap, caplog, app): + permissions.append( + { + "groups": ["testgroup1"], + "testcases": ["testcase1*"], + } + ) + data = brew_build_request_data() + with patch.dict(app.config, {"LDAP_HOST": None}): + r = client.post("/api/v3/results/brew-builds", json=data) + assert r.status_code == 500, r.text + mock_ldap.search_s.assert_not_called() + assert {"message": "Internal Server Error"} == r.json + assert ( + "Internal error: 500 Internal Server Error:" + " LDAP_HOST and LDAP_SEARCHES also need to be defined if PERMISSIONS is defined" + ) in caplog.text + + +def test_api_v3_permission_no_groups_found(client, permissions, mock_ldap, caplog): permissions.append( { "users": ["testuser2"], @@ -277,8 +362,15 @@ def test_api_v3_permission_no_groups_found(client, permissions, mock_ldap): mock_ldap.search_s.return_value = [] data = brew_build_request_data() r = client.post("/api/v3/results/brew-builds", json=data) - assert r.status_code == 401, r.text - assert "Failed to find user testuser1 in LDAP" in r.text + assert r.status_code == 403, r.text + expected_error = ( + "403 Forbidden: User testuser1 is not authorized to submit results" + " for the test case testcase1; failed to find the user in LDAP" + ) + assert expected_error in r.text + assert {"message": ANY} == r.json + assert expected_error == r.json["message"] + assert f"Permission denied: {expected_error}" in caplog.text @pytest.mark.parametrize("params_class", RESULTS_PARAMS_CLASSES)