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

@@ -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

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")
     )

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"

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)