Rework handling of missing state during login

If the authentication state is missing entirely, not incorrect,
during return from authentication, the most likely explanation is
that the user attempted multiple simultaneous logins and then
finished the authentication in another browser window, thus
invalidating the state.

In this case, redirect the user to their destination without further
processing. If they did authentication separately, this is the
(mostly) correct thing to do; they will go to the authenticated
page, although their session might not be as currently as they would
like.

If they're not authenticated, this will restart the authentication
process with new state, which should be the correct thing to do.
There is some risk of a redirect loop, but hopefully we won't go
through that loop more than once, so browsers should cope.

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.

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.

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="[email protected]",
+        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