diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9e92cf149..faf631242 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,7 +2,7 @@ Change log ########## -3.2.0 (unreleased) +3.2.0 (2021-07-14) ================== - Return HTML errors from login failures instead of JSON. @@ -10,6 +10,10 @@ Change log Add a new Helm configuration option, ``config.errorFooter``, that is included in the HTML of any error message that is shown. - Fail authentication and show an error if the user is not a member of any of the groups configured in ``config.groupMapping``. - Revoke the GitHub OAuth authorization if the login fails due to no known groups or an invalid username, since in both cases we want to force GitHub to redo the attribute release. +- HTTP headers are not guaranteed to support character sets other than ASCII, and Starlette forces them to ISO 8859-1. + This interferes with correctly passing the user's full name to protected services via HTTP headers. + Therefore, drop support for sending the user's full name via ``X-Auth-Request-Name``. + The name can still be retrieved from the ``/auth/api/v1/user-info`` API endpoint. 3.1.0 (2021-07-06) ================== diff --git a/docs/applications.rst b/docs/applications.rst index 4872cca3d..7f98a5b3b 100644 --- a/docs/applications.rst +++ b/docs/applications.rst @@ -159,9 +159,6 @@ The value of that annotation is a comma-separated list of desired headers. ``X-Auth-Request-User`` The username of the authenticated user. -``X-Auth-Request-Name`` - The name of the authenticated user, if available. - ``X-Auth-Request-Email`` The email address of the authenticated user, if available. diff --git a/src/gafaelfawr/handlers/auth.py b/src/gafaelfawr/handlers/auth.py index 4c6c67a08..94e92c877 100644 --- a/src/gafaelfawr/handlers/auth.py +++ b/src/gafaelfawr/handlers/auth.py @@ -225,8 +225,6 @@ async def get_auth( X-Auth-Request-Client-Ip The IP address of the client, as determined after parsing ``X-Forwarded-For`` headers. - X-Auth-Request-Name - The full name of the authenticated user, if known. X-Auth-Request-Email The email address of the authenticated user, if known. X-Auth-Request-User @@ -357,8 +355,6 @@ async def build_success_headers( "X-Auth-Request-Token-Scopes": " ".join(sorted(token_data.scopes)), "X-Auth-Request-User": token_data.username, } - if token_data.name: - headers["X-Auth-Request-Name"] = token_data.name if token_data.email: headers["X-Auth-Request-Email"] = token_data.email if token_data.uid: diff --git a/tests/handlers/auth_test.py b/tests/handlers/auth_test.py index d53b2a635..a4156e42d 100644 --- a/tests/handlers/auth_test.py +++ b/tests/handlers/auth_test.py @@ -221,7 +221,6 @@ async def test_success(setup: SetupTest) -> None: assert r.headers["X-Auth-Request-Scopes-Accepted"] == "exec:admin" assert r.headers["X-Auth-Request-Scopes-Satisfy"] == "all" assert r.headers["X-Auth-Request-User"] == token_data.username - assert r.headers["X-Auth-Request-Name"] == token_data.name assert r.headers["X-Auth-Request-Email"] == token_data.email assert r.headers["X-Auth-Request-Uid"] == str(token_data.uid) assert r.headers["X-Auth-Request-Groups"] == "admin" @@ -246,7 +245,6 @@ async def test_success_minimal(setup: SetupTest) -> None: assert r.headers["X-Auth-Request-Scopes-Satisfy"] == "all" assert r.headers["X-Auth-Request-User"] == "user" assert r.headers["X-Auth-Request-Uid"] == "1234" - assert "X-Auth-Request-Name" not in r.headers assert "X-Auth-Request-Email" not in r.headers assert "X-Auth-Request-Groups" not in r.headers @@ -487,3 +485,21 @@ async def test_ajax_unauthorized(setup: SetupTest) -> None: assert not isinstance(authenticate, AuthErrorChallenge) assert authenticate.auth_type == AuthType.Bearer assert authenticate.realm == setup.config.realm + + +@pytest.mark.asyncio +async def test_success_unicode_name(setup: SetupTest) -> None: + user_info = TokenUserInfo(username="user", uid=1234, name="名字") + token_service = setup.factory.create_token_service() + token = await token_service.create_session_token( + user_info, scopes=["read:all"], ip_address="127.0.0.1" + ) + + r = await setup.client.get( + "/auth", + params={"scope": "read:all"}, + headers={"Authorization": f"Bearer {token}"}, + ) + assert r.status_code == 200 + assert r.headers["X-Auth-Request-User"] == "user" + assert r.headers["X-Auth-Request-Uid"] == "1234" diff --git a/tests/handlers/login_github_test.py b/tests/handlers/login_github_test.py index 177e3643b..c46e0b2d4 100644 --- a/tests/handlers/login_github_test.py +++ b/tests/handlers/login_github_test.py @@ -13,18 +13,98 @@ GitHubTeam, GitHubUserInfo, ) -from tests.support.headers import query_from_url from tests.support.logging import parse_log if TYPE_CHECKING: + from typing import Dict, Optional + from _pytest.logging import LogCaptureFixture + from httpx import Response from tests.support.setup import SetupTest +async def simulate_github_login( + setup: SetupTest, + user_info: GitHubUserInfo, + headers: Optional[Dict[str, str]] = None, + return_url: str = "https://example.com/", + paginate_teams: bool = False, + expect_revoke: bool = False, +) -> Response: + """Simulate a GitHub login and return the final response. + + Given the user information that GitHub should return, simulate going to + ``/login`` as an unauthenticated user, following the redirect to GitHub, + and then returning to the ``/login`` handler. + + Parameters + ---------- + setup : `tests.support.setup.SetupTest` + The Gafaelfawr test setup. + user_info : `gafaelfawr.providers.github.GitHubUserInfo` + The user information that GitHub should return. + headers : Dict[`str`, `str`], optional + Optional headers to send on the initial login request. + return_url : `str`, optional + The return URL to pass to the login process. If not provided, a + simple one will be used. + paginate_teams : `bool`, optional + Whether to paginate the team list. If this is set to true, there must + be more then two teams. Default is `False`. + expect_revoke : `bool`, optional + Whether to expect a call from Gafaelfawr to the token revocation URL + immediately after retrieving user information. Default is `False`. + + Returns + ------- + response : ``httpx.Response`` + The response from the return to the ``/login`` handler. + """ + assert setup.config.github + if not headers: + headers = {} + + # Simulate the redirect to GitHub. + setup.set_github_token_response("some-code", "some-github-token") + r = await setup.client.get( + "/login", + params={"rd": return_url}, + headers=headers, + allow_redirects=False, + ) + assert r.status_code == 307 + url = urlparse(r.headers["Location"]) + assert url.scheme == "https" + assert "github.com" in url.netloc + assert url.query + query = parse_qs(url.query) + assert query == { + "client_id": [setup.config.github.client_id], + "scope": [" ".join(GitHubProvider._SCOPES)], + "state": [ANY], + } + + # Simulate the return from GitHub. + setup.set_github_userinfo_response( + "some-github-token", + user_info, + paginate_teams=paginate_teams, + expect_revoke=expect_revoke, + ) + r = await setup.client.get( + "/login", + params={"code": "some-code", "state": query["state"][0]}, + allow_redirects=False, + ) + if r.status_code == 307: + assert r.headers["Location"] == return_url + + return r + + @pytest.mark.asyncio async def test_login(setup: SetupTest, caplog: LogCaptureFixture) -> None: - assert setup.config.github user_info = GitHubUserInfo( name="GitHub User", username="githubuser", @@ -42,23 +122,10 @@ async def test_login(setup: SetupTest, caplog: LogCaptureFixture) -> None: ) return_url = "https://example.com:4444/foo?a=bar&b=baz" - # Simulate the initial authentication request. - setup.set_github_token_response("some-code", "some-github-token") + # Simulate the GitHub login. caplog.clear() - r = await setup.client.get( - "/login", params={"rd": return_url}, allow_redirects=False - ) + r = await simulate_github_login(setup, user_info, return_url=return_url) assert r.status_code == 307 - url = urlparse(r.headers["Location"]) - assert url.scheme == "https" - assert "github.com" in url.netloc - assert url.query - query = parse_qs(url.query) - assert query == { - "client_id": [setup.config.github.client_id], - "scope": [" ".join(GitHubProvider._SCOPES)], - "state": [ANY], - } assert parse_log(caplog) == [ { "event": "Redirecting user to GitHub for authentication", @@ -67,20 +134,7 @@ async def test_login(setup: SetupTest, caplog: LogCaptureFixture) -> None: "path": "/login", "return_url": return_url, "remote": "127.0.0.1", - } - ] - - # Simulate the return from GitHub. - setup.set_github_userinfo_response("some-github-token", user_info) - caplog.clear() - r = await setup.client.get( - "/login", - params={"code": "some-code", "state": query["state"][0]}, - allow_redirects=False, - ) - assert r.status_code == 307 - assert r.headers["Location"] == return_url - assert parse_log(caplog) == [ + }, { "event": "Successfully authenticated user githubuser (123456)", "level": "info", @@ -102,19 +156,34 @@ async def test_login(setup: SetupTest, caplog: LogCaptureFixture) -> None: assert cookie.has_nonstandard_attr("HttpOnly") assert cookie.get_nonstandard_attr("SameSite") == "lax" - # Check that the /auth route works and finds our token. + # Check that the /auth route works and finds our token, and that the user + # information is correct. r = await setup.client.get("/auth", params={"scope": "read:all"}) assert r.status_code == 200 assert r.headers["X-Auth-Request-Token-Scopes"] == "read:all user:token" assert r.headers["X-Auth-Request-Scopes-Accepted"] == "read:all" assert r.headers["X-Auth-Request-Scopes-Satisfy"] == "all" assert r.headers["X-Auth-Request-User"] == "githubuser" - assert r.headers["X-Auth-Request-Name"] == "GitHub User" assert r.headers["X-Auth-Request-Email"] == "githubuser@example.com" assert r.headers["X-Auth-Request-Uid"] == "123456" expected = "org-a-team,org-other-team,other-org-team-with-very--F279yg" assert r.headers["X-Auth-Request-Groups"] == expected + # Do the same verification with the user-info endpoint. + r = await setup.client.get("/auth/api/v1/user-info") + assert r.status_code == 200 + assert r.json() == { + "username": "githubuser", + "name": "GitHub User", + "email": "githubuser@example.com", + "uid": 123456, + "groups": [ + {"name": "org-a-team", "id": 1000}, + {"name": "org-other-team", "id": 1001}, + {"name": "other-org-team-with-very--F279yg", "id": 1002}, + ], + } + @pytest.mark.asyncio async def test_login_redirect_header(setup: SetupTest) -> None: @@ -174,27 +243,12 @@ async def test_cookie_auth_with_token(setup: SetupTest) -> None: teams=[GitHubTeam(slug="a-team", gid=1000, organization="org")], ) - setup.set_github_token_response("some-code", "some-github-token") - r = await setup.client.get( - "/login", - params={"rd": "https://example.com/foo"}, - headers={"Authorization": "token some-jupyterhub-token"}, - allow_redirects=False, - ) - assert r.status_code == 307 - url = urlparse(r.headers["Location"]) - query = parse_qs(url.query) - - # Simulate the return from GitHub. - setup.set_github_userinfo_response("some-github-token", user_info) - r = await setup.client.get( - "/login", - params={"code": "some-code", "state": query["state"][0]}, + # Simulate the GitHub login. + r = await simulate_github_login( + setup, + user_info, headers={"Authorization": "token some-jupyterhub-token"}, - allow_redirects=False, ) - assert r.status_code == 307 - assert r.headers["Location"] == "https://example.com/foo" # Now make a request to the /auth endpoint with a bogus token. r = await setup.client.get( @@ -206,45 +260,6 @@ async def test_cookie_auth_with_token(setup: SetupTest) -> None: assert r.headers["X-Auth-Request-User"] == "githubuser" -@pytest.mark.asyncio -async def test_claim_names(setup: SetupTest) -> None: - """Uses an alternate settings environment with non-default claims.""" - setup.configure(username_claim="username", uid_claim="numeric-uid") - assert setup.config.github - user_info = GitHubUserInfo( - name="GitHub User", - username="githubuser", - uid=123456, - email="githubuser@example.com", - teams=[GitHubTeam(slug="a-team", gid=1000, organization="org")], - ) - - setup.set_github_token_response("some-code", "some-github-token") - r = await setup.client.get( - "/login", - headers={"X-Auth-Request-Redirect": "https://example.com"}, - allow_redirects=False, - ) - assert r.status_code == 307 - url = urlparse(r.headers["Location"]) - query = parse_qs(url.query) - - # Simulate the return from GitHub. - setup.set_github_userinfo_response("some-github-token", user_info) - r = await setup.client.get( - "/login", - params={"code": "some-code", "state": query["state"][0]}, - allow_redirects=False, - ) - assert r.status_code == 307 - - # Check that the /auth route works and sets the headers correctly. - r = await setup.client.get("/auth", params={"scope": "read:all"}) - assert r.status_code == 200 - assert r.headers["X-Auth-Request-User"] == "githubuser" - assert r.headers["X-Auth-Request-Uid"] == "123456" - - @pytest.mark.asyncio async def test_bad_redirect(setup: SetupTest) -> None: user_info = GitHubUserInfo( @@ -252,10 +267,9 @@ async def test_bad_redirect(setup: SetupTest) -> None: username="githubuser", uid=123456, email="githubuser@example.com", - teams=[GitHubTeam(slug="a-team", gid=1000, organization="ORG")], + teams=[GitHubTeam(slug="a-team", gid=1000, organization="org")], ) - setup.set_github_token_response("some-code", "some-github-token") r = await setup.client.get( "/login", params={"rd": "https://foo.example.com/"}, @@ -272,26 +286,16 @@ async def test_bad_redirect(setup: SetupTest) -> None: # But if we're deployed under foo.example.com as determined by the # X-Forwarded-Host header, this will be allowed. - r = await setup.client.get( - "/login", - params={"rd": "https://foo.example.com/"}, + r = await simulate_github_login( + setup, + user_info, headers={ "X-Forwarded-For": "192.168.0.1", "X-Forwarded-Host": "foo.example.com", }, - allow_redirects=False, + return_url="https://foo.example.com/", ) assert r.status_code == 307 - url = urlparse(r.headers["Location"]) - query = parse_qs(url.query) - setup.set_github_userinfo_response("some-github-token", user_info) - r = await setup.client.get( - "/login", - params={"code": "some-code", "state": query["state"][0]}, - allow_redirects=False, - ) - assert r.status_code == 307 - assert r.headers["Location"] == "https://foo.example.com/" @pytest.mark.asyncio @@ -310,23 +314,7 @@ async def test_github_uppercase(setup: SetupTest) -> None: teams=[GitHubTeam(slug="a-team", gid=1000, organization="ORG")], ) - setup.set_github_token_response("some-code", "some-github-token") - r = await setup.client.get( - "/login", - headers={"X-Auth-Request-Redirect": "https://example.com"}, - allow_redirects=False, - ) - assert r.status_code == 307 - url = urlparse(r.headers["Location"]) - query = parse_qs(url.query) - - # Simulate the return from GitHub. - setup.set_github_userinfo_response("some-github-token", user_info) - r = await setup.client.get( - "/login", - params={"code": "some-code", "state": query["state"][0]}, - allow_redirects=False, - ) + r = await simulate_github_login(setup, user_info) assert r.status_code == 307 # The user returned by the /auth route should be forced to lowercase. @@ -348,23 +336,7 @@ async def test_github_admin(setup: SetupTest) -> None: teams=[GitHubTeam(slug="a-team", gid=1000, organization="ORG")], ) - setup.set_github_token_response("some-code", "some-github-token") - r = await setup.client.get( - "/login", - headers={"X-Auth-Request-Redirect": "https://example.com"}, - allow_redirects=False, - ) - assert r.status_code == 307 - url = urlparse(r.headers["Location"]) - query = parse_qs(url.query) - - # Simulate the return from GitHub. - setup.set_github_userinfo_response("some-github-token", user_info) - r = await setup.client.get( - "/login", - params={"code": "some-code", "state": query["state"][0]}, - allow_redirects=False, - ) + r = await simulate_github_login(setup, user_info) assert r.status_code == 307 # The user should have admin:token scope. @@ -383,25 +355,7 @@ async def test_invalid_username(setup: SetupTest) -> None: teams=[GitHubTeam(slug="a-team", gid=1000, organization="ORG")], ) - setup.set_github_token_response("some-code", "some-github-token") - r = await setup.client.get( - "/login", - headers={"X-Auth-Request-Redirect": "https://example.com"}, - allow_redirects=False, - ) - assert r.status_code == 307 - url = urlparse(r.headers["Location"]) - query = parse_qs(url.query) - - # Simulate the return from GitHub. - setup.set_github_userinfo_response( - "some-github-token", user_info, expect_revoke=True - ) - r = await setup.client.get( - "/login", - params={"code": "some-code", "state": query["state"][0]}, - allow_redirects=False, - ) + r = await simulate_github_login(setup, user_info, expect_revoke=True) assert r.status_code == 403 assert "Invalid username: invalid user" in r.text @@ -420,26 +374,11 @@ async def test_invalid_groups(setup: SetupTest) -> None: ], ) - setup.set_github_token_response("some-code", "some-github-token") - r = await setup.client.get( - "/login", - headers={"X-Auth-Request-Redirect": "https://example.com"}, - allow_redirects=False, - ) - assert r.status_code == 307 - url = urlparse(r.headers["Location"]) - query = parse_qs(url.query) - - # Simulate the return from GitHub. - setup.set_github_userinfo_response("some-github-token", user_info) - r = await setup.client.get( - "/login", - params={"code": "some-code", "state": query["state"][0]}, - allow_redirects=False, - ) + r = await simulate_github_login(setup, user_info) assert r.status_code == 307 - # The user returned by the /auth route should be forced to lowercase. + # The invalid groups should not appear but the valid group should still be + # present. r = await setup.client.get("/auth", params={"scope": "read:all"}) assert r.status_code == 200 assert r.headers["X-Auth-Request-Groups"] == "org-a-team" @@ -447,7 +386,6 @@ async def test_invalid_groups(setup: SetupTest) -> None: @pytest.mark.asyncio async def test_paginated_teams(setup: SetupTest) -> None: - assert setup.config.github user_info = GitHubUserInfo( name="GitHub User", username="githubuser", @@ -465,22 +403,7 @@ async def test_paginated_teams(setup: SetupTest) -> None: ], ) - setup.set_github_token_response("some-code", "some-github-token") - r = await setup.client.get( - "/login", params={"rd": "https://example.com"}, allow_redirects=False - ) - assert r.status_code == 307 - query = query_from_url(r.headers["Location"]) - - # Simulate the return from GitHub. - setup.set_github_userinfo_response( - "some-github-token", user_info, paginate_teams=True - ) - r = await setup.client.get( - "/login", - params={"code": "some-code", "state": query["state"][0]}, - allow_redirects=False, - ) + r = await simulate_github_login(setup, user_info, paginate_teams=True) assert r.status_code == 307 # Check the group list. @@ -508,22 +431,7 @@ async def test_no_valid_groups(setup: SetupTest) -> None: teams=[], ) - setup.set_github_token_response("some-code", "some-github-token") - r = await setup.client.get( - "/login", params={"rd": "https://example.com"}, allow_redirects=False - ) - assert r.status_code == 307 - query = query_from_url(r.headers["Location"]) - - # Simulate the return from GitHub. - setup.set_github_userinfo_response( - "some-github-token", user_info, expect_revoke=True - ) - r = await setup.client.get( - "/login", - params={"code": "some-code", "state": query["state"][0]}, - allow_redirects=False, - ) + r = await simulate_github_login(setup, user_info, expect_revoke=True) assert r.status_code == 403 assert r.headers["Cache-Control"] == "no-cache, must-revalidate" assert "githubuser is not a member of any authorized groups" in r.text @@ -532,3 +440,28 @@ async def test_no_valid_groups(setup: SetupTest) -> None: # The user should not be logged in. r = await setup.client.get("/auth", params={"scope": "user:token"}) assert r.status_code == 401 + + +@pytest.mark.asyncio +async def test_unicode_name(setup: SetupTest) -> None: + user_info = GitHubUserInfo( + name="名字", + username="githubuser", + uid=123456, + email="githubuser@example.com", + teams=[GitHubTeam(slug="a-team", gid=1000, organization="org")], + ) + + r = await simulate_github_login(setup, user_info) + assert r.status_code == 307 + + # Check that the name as returned from the user-info API is correct. + r = await setup.client.get("/auth/api/v1/user-info") + assert r.status_code == 200 + assert r.json() == { + "username": "githubuser", + "name": "名字", + "email": "githubuser@example.com", + "uid": 123456, + "groups": [{"name": "org-a-team", "id": 1000}], + } diff --git a/tests/handlers/login_oidc_test.py b/tests/handlers/login_oidc_test.py index b56629db1..f6fab1e6a 100644 --- a/tests/handlers/login_oidc_test.py +++ b/tests/handlers/login_oidc_test.py @@ -103,7 +103,6 @@ async def test_login(setup: SetupTest, caplog: LogCaptureFixture) -> None: assert r.headers["X-Auth-Request-Scopes-Accepted"] == "exec:admin" assert r.headers["X-Auth-Request-Scopes-Satisfy"] == "all" assert r.headers["X-Auth-Request-User"] == token.username - assert r.headers["X-Auth-Request-Name"] == "Some Person" assert r.headers["X-Auth-Request-Email"] == "person@example.com" assert r.headers["X-Auth-Request-Uid"] == str(token.uid) assert r.headers["X-Auth-Request-Groups"] == "admin" @@ -165,6 +164,44 @@ async def test_oauth2_callback(setup: SetupTest) -> None: assert r.headers["Location"] == return_url +@pytest.mark.asyncio +async def test_claim_names(setup: SetupTest) -> None: + """Uses an alternate settings environment with non-default claims.""" + setup.configure("oidc", username_claim="username", uid_claim="numeric_uid") + assert setup.config.oidc + token = setup.create_upstream_oidc_token( + groups=["admin"], username="alt-username", numeric_uid=7890 + ) + setup.set_oidc_token_response("some-code", token) + setup.set_oidc_configuration_response(setup.config.issuer.keypair) + return_url = "https://example.com/foo" + + r = await setup.client.get( + "/login", params={"rd": return_url}, allow_redirects=False + ) + assert r.status_code == 307 + url = urlparse(r.headers["Location"]) + query = parse_qs(url.query) + assert query["redirect_uri"][0] == setup.config.oidc.redirect_url + + # Simulate the return from the OpenID Connect provider. + r = await setup.client.get( + "/login", + params={"code": "some-code", "state": query["state"][0]}, + allow_redirects=False, + ) + assert r.status_code == 307 + assert r.headers["Location"] == return_url + + # Check that the /auth route works and sets the headers correctly. uid + # will be set to some-user and uidNumber will be set to 1000, so we'll + # know if we read the alternate claim names correctly instead. + r = await setup.client.get("/auth", params={"scope": "read:all"}) + assert r.status_code == 200 + assert r.headers["X-Auth-Request-User"] == "alt-username" + assert r.headers["X-Auth-Request-Uid"] == "7890" + + @pytest.mark.asyncio async def test_callback_error( setup: SetupTest, caplog: LogCaptureFixture @@ -480,3 +517,39 @@ async def test_no_valid_groups(setup: SetupTest) -> None: # The user should not be logged in. r = await setup.client.get("/auth", params={"scope": "user:token"}) assert r.status_code == 401 + + +@pytest.mark.asyncio +async def test_unicode_name(setup: SetupTest) -> None: + setup.configure("oidc") + token = setup.create_upstream_oidc_token(name="名字", groups=["admin"]) + setup.set_oidc_token_response("some-code", token) + setup.set_oidc_configuration_response(setup.config.issuer.keypair) + return_url = "https://example.com/foo" + + r = await setup.client.get( + "/login", params={"rd": return_url}, allow_redirects=False + ) + assert r.status_code == 307 + url = urlparse(r.headers["Location"]) + query = parse_qs(url.query) + + # Simulate the return from the OpenID Connect provider. + r = await setup.client.get( + "/login", + params={"code": "some-code", "state": query["state"][0]}, + allow_redirects=False, + ) + assert r.status_code == 307 + assert r.headers["Location"] == return_url + + # Check that the name as returned from the user-info API is correct. + r = await setup.client.get("/auth/api/v1/user-info") + assert r.status_code == 200 + assert r.json() == { + "username": token.username, + "name": "名字", + "email": token.claims["email"], + "uid": token.uid, + "groups": [{"name": "admin", "id": 1000}], + }