diff --git a/changelog.d/20240919_172648_rra_DM_46399.md b/changelog.d/20240919_172648_rra_DM_46399.md new file mode 100644 index 000000000..55a9e6275 --- /dev/null +++ b/changelog.d/20240919_172648_rra_DM_46399.md @@ -0,0 +1,3 @@ +### Bug fixes + +- If the user returns from authentication and no longer has login state in their cookie, redirect them to the destination URL without further processing instead of returning an authentication state mismatch error. The most likely cause of this state is that the user authenticated from another browser tab while this authentication is pending, so Gafaelfawr should use their existing token or restart the authentication process. diff --git a/src/gafaelfawr/handlers/login.py b/src/gafaelfawr/handlers/login.py index 2b0a572ee..a07a8e0f3 100644 --- a/src/gafaelfawr/handlers/login.py +++ b/src/gafaelfawr/handlers/login.py @@ -42,8 +42,8 @@ class LoginError(Enum): PROVIDER_FAILED = "Authentication provider failed" PROVIDER_NETWORK = "Cannot contact authentication provider" RETURN_URL_MISSING = "Invalid state: return_url not present in cookie" - STATE_INVALID = "Authentication state mismatch" - STATE_MISSING = "No authentication state" + STATE_INVALID = "Authentication state mismatch, please start over" + STATE_MISSING = "No authentication state, please start over" @router.get( @@ -398,6 +398,11 @@ async def _construct_login_response( Handles the target of the redirect back from an external authentication provider with new authentication state information. + If there is no authentication state in the user's cookie, it is likely + that the user was attempting logins in multiple tabs and already logged in + via some other tab. Redirect the user to their destination, which in the + worst case will just restart the authentication with proper state. + Parameters ---------- code @@ -428,12 +433,16 @@ async def _construct_login_response( Raised if there is some problem retrieving information from the authentication provider. """ - if state != context.state.state: - return _error_user(context, LoginError.STATE_INVALID) return_url = context.state.return_url if not return_url: return _error_user(context, LoginError.RETURN_URL_MISSING) context.rebind_logger(return_url=return_url) + if not context.state.state: + msg = "Login state missing, redirecting without authentication" + context.logger.info(msg) + return RedirectResponse(return_url) + if state != context.state.state: + return _error_user(context, LoginError.STATE_INVALID) # Retrieve the user identity and authorization information based on the # reply from the authentication provider, and construct a token. diff --git a/tests/handlers/login_github_test.py b/tests/handlers/login_github_test.py index d9158b300..37dc14fc5 100644 --- a/tests/handlers/login_github_test.py +++ b/tests/handlers/login_github_test.py @@ -2,6 +2,8 @@ from __future__ import annotations +import base64 +import os from unittest.mock import ANY from urllib.parse import parse_qs, urlparse @@ -11,11 +13,14 @@ from safir.testing.slack import MockSlackWebhook from gafaelfawr.config import Config +from gafaelfawr.constants import COOKIE_NAME from gafaelfawr.dependencies.config import config_dependency from gafaelfawr.factory import Factory from gafaelfawr.models.github import GitHubTeam, GitHubUserInfo +from gafaelfawr.models.state import State from gafaelfawr.providers.github import GitHubProvider +from ..support.constants import TEST_HOSTNAME from ..support.github import mock_github from ..support.logging import parse_log @@ -546,3 +551,48 @@ async def test_unicode_name( {"name": "org-a-team", "id": 1000}, ], } + + +@pytest.mark.asyncio +async def test_invalid_state( + client: AsyncClient, config: Config, respx_mock: respx.Router +) -> None: + user_info = GitHubUserInfo( + name="GitHub User", + username="githubuser", + uid=123456, + email="githubuser@example.com", + teams=[], + ) + return_url = "https://example.com/foo" + + mock_github(respx_mock, "some-code", user_info) + r = await client.get("/login", params={"rd": return_url}) + assert r.status_code == 307 + url = urlparse(r.headers["Location"]) + query = parse_qs(url.query) + + # Change the state to something that won't match. + state = await State.from_cookie(r.cookies[COOKIE_NAME]) + state.state = base64.urlsafe_b64encode(os.urandom(16)).decode() + client.cookies.set(COOKIE_NAME, state.to_cookie(), domain=TEST_HOSTNAME) + + # We should now get an error from the login endpoint. + r = await client.get( + "/login", params={"code": "some-code", "state": query["state"][0]} + ) + assert r.status_code == 403 + assert "Authentication state mismatch" in r.text + + # Change the state to None. + state.state = None + client.cookies.set(COOKIE_NAME, state.to_cookie(), domain=TEST_HOSTNAME) + + # Now we should get a simple redirect to the return URL even though the + # authentication isn't complete, since the code should assume, given the + # empty state, that the user may have logged in via another window. + r = await client.get( + "/login", params={"code": "some-code", "state": query["state"][0]} + ) + assert r.status_code == 307 + assert r.headers["Location"] == return_url