From 8234217168b8cc0ebea1a0ff27c5865b48887a5f Mon Sep 17 00:00:00 2001 From: Mark Boyd Date: Mon, 4 Nov 2024 15:51:14 -0500 Subject: [PATCH 1/4] fix check for whether user is admin to raise unexpected error --- cf_auth_proxy/uaa.py | 5 +---- tests/unit/test_uaa.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/cf_auth_proxy/uaa.py b/cf_auth_proxy/uaa.py index 0c7a268..e15654f 100644 --- a/cf_auth_proxy/uaa.py +++ b/cf_auth_proxy/uaa.py @@ -32,10 +32,7 @@ def is_user_cf_admin(user_id, token): s.headers["Authorization"] = f"Bearer {token}" url = urljoin(config.UAA_BASE_URL, f"Users/{user_id}") response = s.get(url) - try: - response.raise_for_status() - except: - return "Unexpected error", 500 + response.raise_for_status() data = response.json() user_groups = [group["display"] for group in data.get("groups", [])] return config.CF_ADMIN_GROUP_NAME in user_groups diff --git a/tests/unit/test_uaa.py b/tests/unit/test_uaa.py index 80433e6..7879e5f 100644 --- a/tests/unit/test_uaa.py +++ b/tests/unit/test_uaa.py @@ -1,3 +1,4 @@ +import pytest import requests_mock from cf_auth_proxy import uaa @@ -33,3 +34,17 @@ def test_user_has_no_groups(uaa_user_has_no_groups_response): isAdmin = uaa.is_user_cf_admin("a-user-id", "a-token") assert not isAdmin assert m.last_request._request.headers["Authorization"] == "Bearer a-token" + + +def test_uaa_has_unexpected_error(): + with requests_mock.Mocker() as m: + m.get("http://mock.uaa/Users/a-user-id", status_code=500) + with pytest.raises(Exception): + uaa.is_user_cf_admin("a-user-id", "a-token") + + +def test_uaa_has_unauthorized_error(): + with requests_mock.Mocker() as m: + m.get("http://mock.uaa/Users/a-user-id", status_code=401) + with pytest.raises(Exception): + uaa.is_user_cf_admin("a-user-id", "a-token") From 78a7a4e099a5ba7c298e159632b4011081f86fea Mon Sep 17 00:00:00 2001 From: Mark Boyd Date: Mon, 4 Nov 2024 15:53:54 -0500 Subject: [PATCH 2/4] add check to ensure admin role is only returned if is_cf_admin is literally True --- cf_auth_proxy/app.py | 2 +- tests/unit/test_app.py | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/cf_auth_proxy/app.py b/cf_auth_proxy/app.py index af4d67b..4042e63 100644 --- a/cf_auth_proxy/app.py +++ b/cf_auth_proxy/app.py @@ -175,7 +175,7 @@ def redirect_to_auth(): # allowed path if session.get("user_id"): headers["x-proxy-user"] = session["email"] - roles = [("admin" if session.get("is_cf_admin") else "user")] + roles = [("admin" if session.get("is_cf_admin") == True else "user")] roles += session.get("user_orgs", []) headers["x-proxy-roles"] = ",".join(roles) diff --git a/tests/unit/test_app.py b/tests/unit/test_app.py index adfeb2f..6f75a0e 100644 --- a/tests/unit/test_app.py +++ b/tests/unit/test_app.py @@ -144,3 +144,14 @@ def test_does_not_modify_existing_xff_lowercase(client): s["email"] = "me" client.get("/home", headers={"x-forwarded-for": "y.y.y.y"}) assert m.last_request._request.headers["X-Forwarded-For"] == "y.y.y.y" + + +def test_does_not_accept_truthy_is_cf_admin(client): + with requests_mock.Mocker() as m: + m.get("http://mock.dashboard/home") + with client.session_transaction() as s: + s["user_id"] = "me2" + s["is_cf_admin"] = "truthy" + s["email"] = "me" + client.get("/home") + assert "admin" not in m.last_request._request.headers["x-proxy-roles"] From 98fbfa6720778d5256d75ddff014158dfedd0a28 Mon Sep 17 00:00:00 2001 From: Mark Boyd Date: Mon, 4 Nov 2024 16:00:28 -0500 Subject: [PATCH 3/4] update get_client_credentials_token to raise error on unexpected status code --- cf_auth_proxy/uaa.py | 6 +----- tests/unit/test_uaa.py | 7 +++++++ 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/cf_auth_proxy/uaa.py b/cf_auth_proxy/uaa.py index e15654f..04fa8a5 100644 --- a/cf_auth_proxy/uaa.py +++ b/cf_auth_proxy/uaa.py @@ -19,11 +19,7 @@ def get_client_credentials_token(): timeout=config.REQUEST_TIMEOUT, ) - try: - response.raise_for_status() - except: - return "Unexpected error", 500 - + response.raise_for_status() return response.json()["access_token"] diff --git a/tests/unit/test_uaa.py b/tests/unit/test_uaa.py index 7879e5f..911900c 100644 --- a/tests/unit/test_uaa.py +++ b/tests/unit/test_uaa.py @@ -48,3 +48,10 @@ def test_uaa_has_unauthorized_error(): m.get("http://mock.uaa/Users/a-user-id", status_code=401) with pytest.raises(Exception): uaa.is_user_cf_admin("a-user-id", "a-token") + + +def test_uaa_get_client_credentials_token_raises_unexpected_error(): + with requests_mock.Mocker() as m: + m.post("http://mock.uaa/token", status_code=500) + with pytest.raises(Exception): + uaa.get_client_credentials_token() From 1c3e1fec4112362f1654018adafe7f12fb2fad4b Mon Sep 17 00:00:00 2001 From: Mark Boyd Date: Mon, 4 Nov 2024 16:22:44 -0500 Subject: [PATCH 4/4] add test for callback returning error on UAA token failure --- tests/unit/test_app.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/unit/test_app.py b/tests/unit/test_app.py index 6f75a0e..15390c6 100644 --- a/tests/unit/test_app.py +++ b/tests/unit/test_app.py @@ -155,3 +155,14 @@ def test_does_not_accept_truthy_is_cf_admin(client): s["email"] = "me" client.get("/home") assert "admin" not in m.last_request._request.headers["x-proxy-roles"] + + +def test_callback_returns_error_on_uaa_token_failure(client): + with requests_mock.Mocker() as m: + m.post("http://mock.uaa/token", status_code=401) + with client.session_transaction() as s: + s["user_id"] = "me2" + s["email"] = "me" + s["state"] = "foo" + response = client.get("/cb?code=fakecode&state=foo") + assert response.status_code == 500