From c8b4b0ce9a88a17a0faedfdbd92c961d62738a48 Mon Sep 17 00:00:00 2001 From: "FanaticPythoner (Nathan Trudeau)" Date: Mon, 27 Apr 2026 16:01:36 -0400 Subject: [PATCH] Handle grant scope mismatches in login Detect the Gitea different scope authorize failure as a dedicated auth error, show the revoke URL and client ID, and retry login once after manual grant revocation without forcing a second full authentication. Expand the requested scope set to include read:organization, add the revoke-grant helper path and setup auto-yes flag in the scaffold, document the recovery flow, and cover revoke prompting and retry behavior in forge_auth tests. --- Justfile | 4 + README.md | 1 + scripts/forge_auth.py | 149 ++++++++++++++++++++-- scripts/revoke_grant.sh | 51 ++++++++ scripts/setup.sh | 10 +- tests/test_forge_auth.py | 258 ++++++++++++++++++++++++++++++++++++++- 6 files changed, 458 insertions(+), 15 deletions(-) create mode 100755 scripts/revoke_grant.sh diff --git a/Justfile b/Justfile index 5b99bca..2718a8f 100644 --- a/Justfile +++ b/Justfile @@ -71,6 +71,10 @@ logout: relogin: @bash scripts/forge_login.sh --force +# Open Gitea's "Authorized OAuth2 Applications" page to revoke a stale grant. Resolves the "different scope" failure mode (see docs/oauth-grant-scope-mismatch.md). +revoke-grant: + @bash scripts/revoke_grant.sh + # Force a token refresh (normally automatic inside the credential helper). refresh: @python3 scripts/forge_auth.py refresh --force diff --git a/README.md b/README.md index f21cc1a..d34b09d 100644 --- a/README.md +++ b/README.md @@ -217,6 +217,7 @@ with a browser to populate a valid refresh token before running | Git prompts for a password on pull/push | Refresh token expired. Run `just relogin`. | | `just status` shows `live: False` | Run `just refresh`; also happens automatically on the next git op. | | `just clone-orchestrator` prints `already cloned` | Intended; idempotent. | +| `just login` exits with `Gitea server_error: "a grant exists with different scope"` | Run `just revoke-grant` (opens `/user/settings/applications` and prints the matching `FSDGG_CLI_CLIENT_ID`). Revoke the matching app, then re-run `just login`. Required only once after a scope-set change. Full reference: `docs/oauth-grant-scope-mismatch.md`. | | Reset local state | `just uninstall`. | ## Security properties diff --git a/scripts/forge_auth.py b/scripts/forge_auth.py index fe217e4..d77c01b 100755 --- a/scripts/forge_auth.py +++ b/scripts/forge_auth.py @@ -52,6 +52,42 @@ class AuthError(RuntimeError): """Any non-transient OAuth/auth-file error. Always user-visible.""" +class ScopeMismatchAuthError(AuthError): + """Gitea ``server_error`` caused by a grant/scope conflict. + + Raised when Gitea refuses an authorize request because a grant + already exists for ``client_id`` under a different scope set + (RFC 6749 §4.1.2.1; ``error_description`` contains "different + scope"). Recovery: revoke the grant under + ``/user/settings/applications`` and re-run. + + Subclassing ``AuthError`` keeps existing ``except AuthError`` + handlers (e.g., the credential helper) unchanged. Callers that + want to drive an interactive revoke-and-retry flow check for + this concrete subclass and read the structured attributes. + """ + + def __init__( + self, + message: str, + *, + gitea_base_url: str, + client_id: str, + scopes: str, + ) -> None: + super().__init__(message) + self.gitea_base_url = gitea_base_url + self.client_id = client_id + self.scopes = scopes + + @property + def revoke_url(self) -> str: + base = self.gitea_base_url.rstrip("/") + if not base: + return "/user/settings/applications" + return f"{base}/user/settings/applications" + + # -------------------------------------------------------------------- # CLI output helpers. Tag scheme matches scripts/common.sh: # cyan=info, green=ok, yellow=warn, red=err. ANSI disabled when stderr @@ -164,7 +200,7 @@ class ForgeAuthConfig: gitea_base_url: str client_id: str redirect_uri: str - scopes: str = "openid profile email read:user read:repository write:repository" + scopes: str = "openid profile email read:user read:organization read:repository write:repository" insecure_tls: bool = False # only for dev Gitea with self-signed certs expected_username: str = "" # from FORGE_GITEA_USERNAME; empty = no check @@ -308,9 +344,19 @@ def build_authorize_url( *, challenge: str, state: str, + force_login_prompt: bool = True, ) -> str: - """PKCE authorise URL with ``prompt=login`` (OIDC Core §3.1.2.1) and, - when ``config.expected_username`` is set, ``login_hint``. + """PKCE authorise URL with optional ``prompt=login`` and ``login_hint``. + + ``prompt=login`` (OIDC Core §3.1.2.1) is the default: it forces + Gitea to re-authenticate the user even when a session cookie is + already present, which is the right ergonomic for the first attempt + of ``just login``. Setting ``force_login_prompt=False`` drops the + parameter so the second attempt of an auto-retry (after a grant + revocation) reuses the established session cookie and only triggers + the consent screen, halving the browser-side prompts. The post-auth + ``expected_username`` check in ``run_login`` still enforces the + wrong-user guard on either path. ``login_hint`` is unaffected. """ params: dict[str, str] = { "client_id": config.client_id, @@ -320,8 +366,9 @@ def build_authorize_url( "state": state, "code_challenge": challenge, "code_challenge_method": "S256", - "prompt": "login", } + if force_login_prompt: + params["prompt"] = "login" if config.expected_username: params["login_hint"] = config.expected_username return f"{endpoints['authorization_endpoint']}?{urlencode(params)}" @@ -500,13 +547,16 @@ def _build_authorize_error( if "different scope" in low or ("scope" in low and "grant" in low): cid = (client_id or "").strip() scope_fragment = scopes or "" - return AuthError( + return ScopeMismatchAuthError( f'Gitea server_error: "{desc or error}".\n' f" Client ID: {cid}\n" f" Revoke at: {settings_url} " f"(\"Authorized OAuth2 Applications\").\n" f" Requested: {scope_fragment}\n" - " See: docs/oauth-grant-scope-mismatch.md" + " See: docs/oauth-grant-scope-mismatch.md", + gitea_base_url=gitea_base_url, + client_id=cid, + scopes=scope_fragment, ) if error == "access_denied" or "access_denied" in low or "denied" in low: @@ -800,12 +850,16 @@ def run_login( open_browser: bool = True, force: bool = False, print_authorize_url: bool = True, + force_login_prompt: bool = True, ) -> AuthFile: """Run the full PKCE flow and persist the result. Idempotent: if the stored file already carries a live Gitea token and ``force`` is False, skip everything and return the existing state. The caller is the one deciding when to force a refresh. + ``force_login_prompt`` is forwarded to ``build_authorize_url`` and + is set to False by the auto-retry path after a grant revocation so + Gitea reuses the existing browser session. """ store = auth_store_path() existing = AuthFile.read(store) @@ -818,7 +872,13 @@ def run_login( nonce = secrets.token_urlsafe(24) state = sign_state(session_key, nonce) - auth_url = build_authorize_url(config, endpoints, challenge=challenge, state=state) + auth_url = build_authorize_url( + config, + endpoints, + challenge=challenge, + state=state, + force_login_prompt=force_login_prompt, + ) redirect = urlparse(config.redirect_uri) assert redirect.hostname and redirect.port # validated upstream @@ -964,11 +1024,84 @@ def run_logout() -> Path | None: # -------------------------------------------------------------------- +def _can_prompt_for_revoke() -> bool: + """Return True only when the environment is interactive enough to + block on ``input()`` and have the contributor click "Revoke". + + Disabling vectors (any one of these returns False): + + * ``FORGE_AUTO_REVOKE`` set to ``0``/``no``/``false``: explicit + opt-out for callers that want strict fail-fast behaviour. + * ``FORGE_SETUP_YES=1``: non-interactive auto-yes mode used by + ``setup.sh`` and CI; never block on ``input()``. + * stderr or stdin is not a TTY: avoids deadlocks in piped or + backgrounded executions. + """ + val = os.environ.get("FORGE_AUTO_REVOKE", "1").strip().lower() + if val in {"0", "no", "false"}: + return False + if os.environ.get("FORGE_SETUP_YES", "0").strip() == "1": + return False + try: + return sys.stderr.isatty() and sys.stdin.isatty() + except (AttributeError, ValueError): + return False + + +def _prompt_revoke_and_wait( + exc: ScopeMismatchAuthError, *, open_browser: bool +) -> bool: + """Drive the manual revoke step. Returns True iff the operator + pressed Enter (i.e., agreed to retry); False on EOF/Ctrl-C. + + Side effects: prints the revoke URL and ``client_id`` to stderr; + when ``open_browser`` is True, also calls ``webbrowser.open`` on + the revoke URL (best-effort; failure is silent). + """ + cli_info( + 'opening Gitea\'s "Authorized OAuth2 Applications" page so the ' + "conflicting grant can be revoked." + ) + cli_info(f" URL: {exc.revoke_url}") + cli_info(f" Client ID: {exc.client_id}") + if open_browser: + try: + webbrowser.open(exc.revoke_url, new=2) + except webbrowser.Error: + pass + cli_info( + 'after clicking "Revoke" on the row matching the Client ID ' + "above, press Enter here to retry login (or Ctrl-C to abort)." + ) + try: + input("") + except (EOFError, KeyboardInterrupt): + return False + return True + + def _cmd_login(argv: list[str]) -> int: force = "--force" in argv no_browser = "--no-browser" in argv config = ForgeAuthConfig.from_env() - state = run_login(config, open_browser=not no_browser, force=force) + try: + state = run_login(config, open_browser=not no_browser, force=force) + except ScopeMismatchAuthError as exc: + cli_err(str(exc)) + if not _can_prompt_for_revoke(): + return 1 + if not _prompt_revoke_and_wait(exc, open_browser=not no_browser): + return 1 + # Single retry. ``force=True`` bypasses the live-token short- + # circuit; ``force_login_prompt=False`` reuses the browser + # session cookie established by the failed first attempt so + # Gitea only shows the consent screen on the retry. + state = run_login( + config, + open_browser=not no_browser, + force=True, + force_login_prompt=False, + ) cli_ok(f"authenticated as: {state.username}") cli_info(f"auth file: {auth_store_path()}") return 0 diff --git a/scripts/revoke_grant.sh b/scripts/revoke_grant.sh new file mode 100755 index 0000000..fc898f3 --- /dev/null +++ b/scripts/revoke_grant.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# +# revoke_grant.sh: open Gitea's "Authorized OAuth2 Applications" page so +# the contributor can revoke a stale OAuth grant whose scope set no +# longer matches the unified scope set requested by this scaffold and +# the orchestrator's gateway. See docs/oauth-grant-scope-mismatch.md +# for the full failure mode and recovery procedure. + +set -euo pipefail + +here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" +# shellcheck disable=SC1091 +. "$here/common.sh" + +load_env +require_env FORGE_GITEA_URL +require_env FSDGG_CLI_CLIENT_ID +require_cmd python3 + +base="${FORGE_GITEA_URL%/}" +url="${base}/user/settings/applications" +cid="${FSDGG_CLI_CLIENT_ID}" + +cat < None: out = self._capture({"SSH_CONNECTION": "1.2.3.4 22 5.6.7.8 22", "USER": "alice"}) - self.assertIn("running inside an SSH session", out) + self.assertIn("SSH session detected", out) def test_non_ssh_branch_when_no_ssh_env(self) -> None: out = self._capture() - self.assertIn("if this machine is remote", out) - self.assertNotIn("running inside an SSH session", out) + self.assertIn("Remote-host case", out) + self.assertNotIn("SSH session detected", out) class BuildAuthorizeErrorTests(unittest.TestCase): @@ -584,7 +584,7 @@ class BuildAuthorizeErrorTests(unittest.TestCase): BASE = "https://gitea.example.com" CID = "ba4ec9ec-8ae8-4450-9cec-fd532bbe63d5" - SCOPES = "openid profile email read:user read:repository write:repository" + SCOPES = "openid profile email read:user read:organization read:repository write:repository" def _exc_different_scope(self, **overrides: str) -> fa.AuthError: kw: dict[str, object] = { @@ -724,6 +724,256 @@ class AuthorizeUrlIsHeadlessInvariantTests(unittest.TestCase): params = parse_qs(urlparse(url).query) self.assertEqual(params["scope"], [self.cfg.scopes]) + def test_default_url_carries_prompt_login(self) -> None: + from urllib.parse import parse_qs, urlparse + + url = fa.build_authorize_url( + self.cfg, self.endpoints, challenge="c", state="s" + ) + params = parse_qs(urlparse(url).query) + self.assertEqual(params["prompt"], ["login"]) + + def test_force_login_prompt_false_drops_prompt_param(self) -> None: + from urllib.parse import parse_qs, urlparse + + url = fa.build_authorize_url( + self.cfg, + self.endpoints, + challenge="c", + state="s", + force_login_prompt=False, + ) + params = parse_qs(urlparse(url).query) + self.assertNotIn("prompt", params) + # login_hint must still ride along; the retry path keeps it so + # Gitea can pre-fill the username field if a fresh login screen + # ever does appear (e.g., expired session cookie). + self.assertEqual(params["login_hint"], [self.cfg.expected_username]) + + +class ScopeMismatchAuthErrorTests(unittest.TestCase): + """Contract tests for the ``ScopeMismatchAuthError`` subclass. + + The "different scope" branch of ``_build_authorize_error`` must + return a ``ScopeMismatchAuthError`` so callers can drive a + revoke-and-retry recovery flow instead of swallowing the error. + The class is also a subclass of ``AuthError`` so existing + ``except AuthError`` handlers (e.g., the credential helper) keep + working unchanged. + """ + + BASE = "https://gitea.example.com" + CID = "ba4ec9ec-8ae8-4450-9cec-fd532bbe63d5" + SCOPES = "openid profile email read:user read:organization read:repository write:repository" + + def _build(self, **overrides): + kw = { + "error": "server_error", + "error_description": "a grant exists with different scope", + "gitea_base_url": self.BASE, + "client_id": self.CID, + "scopes": self.SCOPES, + } + kw.update(overrides) + return fa._build_authorize_error( + str(kw["error"]), + str(kw["error_description"]) if kw["error_description"] else None, + str(kw["gitea_base_url"]), + client_id=str(kw["client_id"]), + scopes=str(kw["scopes"]), + ) + + def test_different_scope_returns_subclass(self) -> None: + self.assertIsInstance(self._build(), fa.ScopeMismatchAuthError) + + def test_subclass_inherits_from_autherror(self) -> None: + self.assertTrue(issubclass(fa.ScopeMismatchAuthError, fa.AuthError)) + + def test_attributes_carry_diagnostic_fields(self) -> None: + exc = self._build() + self.assertEqual(exc.gitea_base_url, self.BASE) + self.assertEqual(exc.client_id, self.CID) + self.assertEqual(exc.scopes, self.SCOPES) + + def test_revoke_url_with_base(self) -> None: + exc = self._build() + self.assertEqual( + exc.revoke_url, + f"{self.BASE}/user/settings/applications", + ) + + def test_revoke_url_strips_trailing_slash(self) -> None: + exc = self._build(gitea_base_url=self.BASE + "/") + self.assertEqual( + exc.revoke_url, + f"{self.BASE}/user/settings/applications", + ) + + def test_revoke_url_without_base_uses_placeholder(self) -> None: + exc = self._build(gitea_base_url="") + self.assertEqual( + exc.revoke_url, + "/user/settings/applications", + ) + + def test_access_denied_branch_is_plain_autherror(self) -> None: + # Negative case: only the scope-conflict branch upgrades to + # the subclass; access_denied stays a generic AuthError. + exc = fa._build_authorize_error( + "access_denied", "denied by user", self.BASE, + ) + self.assertNotIsInstance(exc, fa.ScopeMismatchAuthError) + self.assertIsInstance(exc, fa.AuthError) + + +class CanPromptForRevokeTests(unittest.TestCase): + """``_can_prompt_for_revoke`` gates the interactive retry path. + + Returns False whenever the contributor cannot reasonably be asked + to press Enter: explicit opt-out via ``FORGE_AUTO_REVOKE``, + non-interactive mode via ``FORGE_SETUP_YES``, or non-TTY stdio. + """ + + def setUp(self) -> None: + self._env_keys = ("FORGE_AUTO_REVOKE", "FORGE_SETUP_YES") + self._env_backup = {k: os.environ.get(k) for k in self._env_keys} + for k in self._env_keys: + os.environ.pop(k, None) + + def tearDown(self) -> None: + for k, v in self._env_backup.items(): + if v is None: + os.environ.pop(k, None) + else: + os.environ[k] = v + + def _run(self, *, stderr_tty: bool = True, stdin_tty: bool = True) -> bool: + with mock.patch.object(sys.stderr, "isatty", lambda: stderr_tty), \ + mock.patch.object(sys.stdin, "isatty", lambda: stdin_tty): + return fa._can_prompt_for_revoke() + + def test_default_with_both_ttys_returns_true(self) -> None: + self.assertTrue(self._run()) + + def test_force_auto_revoke_off_disables(self) -> None: + for v in ("0", "no", "false", "FALSE", "No"): + with self.subTest(value=v): + os.environ["FORGE_AUTO_REVOKE"] = v + self.assertFalse(self._run()) + os.environ.pop("FORGE_AUTO_REVOKE", None) + + def test_setup_yes_disables(self) -> None: + os.environ["FORGE_SETUP_YES"] = "1" + self.assertFalse(self._run()) + + def test_no_stderr_tty_disables(self) -> None: + self.assertFalse(self._run(stderr_tty=False)) + + def test_no_stdin_tty_disables(self) -> None: + self.assertFalse(self._run(stdin_tty=False)) + + +class CmdLoginRetryOnScopeMismatchTests(unittest.TestCase): + """``_cmd_login`` auto-retries once after ``ScopeMismatchAuthError`` + when the prompt path is enabled; otherwise exits 1 immediately. + + The retry must call ``run_login`` with ``force=True`` and + ``force_login_prompt=False`` so the cached live-token short-circuit + cannot mask a stale grant and Gitea can reuse the existing browser + session cookie (only consent screen on retry). + """ + + def setUp(self) -> None: + # Capture stderr to keep cli_err/cli_ok/cli_info from polluting + # the test runner output for the entire class. + stderr_patch = mock.patch.object(sys, "stderr", new_callable=io.StringIO) + stderr_patch.start() + self.addCleanup(stderr_patch.stop) + self.fake_cfg = fa.ForgeAuthConfig( + gitea_base_url="https://gitea.example.com", + client_id="ba4ec9ec-8ae8-4450-9cec-fd532bbe63d5", + redirect_uri="http://127.0.0.1:38111/callback", + ) + self.fake_state = mock.Mock(username="alice") + self.scope_exc = fa.ScopeMismatchAuthError( + "boom", + gitea_base_url=self.fake_cfg.gitea_base_url, + client_id=self.fake_cfg.client_id, + scopes=self.fake_cfg.scopes, + ) + + def _common_patches(self, *, run_login_side_effect): + return [ + mock.patch.object( + fa.ForgeAuthConfig, "from_env", return_value=self.fake_cfg + ), + mock.patch.object( + fa, "run_login", side_effect=run_login_side_effect + ), + mock.patch.object( + fa, "auth_store_path", return_value=Path("/tmp/dummy.json") + ), + ] + + def _start(self, patches): + for p in patches: + p.start() + self.addCleanup(lambda: [p.stop() for p in patches]) + + def test_retries_when_prompt_allowed(self) -> None: + patches = self._common_patches( + run_login_side_effect=[self.scope_exc, self.fake_state] + ) + [ + mock.patch.object(fa, "_can_prompt_for_revoke", return_value=True), + mock.patch.object(fa, "_prompt_revoke_and_wait", return_value=True), + ] + self._start(patches) + rc = fa._cmd_login([]) + self.assertEqual(rc, 0) + self.assertEqual(fa.run_login.call_count, 2) + _, kwargs = fa.run_login.call_args_list[1] + self.assertTrue(kwargs.get("force")) + # The retry must drop ``prompt=login`` so Gitea reuses the + # browser session cookie established by the failed first call. + self.assertIs(kwargs.get("force_login_prompt"), False) + + def test_does_not_retry_when_prompt_disabled(self) -> None: + prompt_mock = mock.Mock(return_value=True) + patches = self._common_patches( + run_login_side_effect=[self.scope_exc] + ) + [ + mock.patch.object(fa, "_can_prompt_for_revoke", return_value=False), + mock.patch.object(fa, "_prompt_revoke_and_wait", new=prompt_mock), + ] + self._start(patches) + rc = fa._cmd_login([]) + self.assertEqual(rc, 1) + self.assertEqual(fa.run_login.call_count, 1) + prompt_mock.assert_not_called() + + def test_does_not_retry_when_user_aborts(self) -> None: + patches = self._common_patches( + run_login_side_effect=[self.scope_exc] + ) + [ + mock.patch.object(fa, "_can_prompt_for_revoke", return_value=True), + mock.patch.object(fa, "_prompt_revoke_and_wait", return_value=False), + ] + self._start(patches) + rc = fa._cmd_login([]) + self.assertEqual(rc, 1) + self.assertEqual(fa.run_login.call_count, 1) + + def test_unrelated_autherror_propagates(self) -> None: + patches = self._common_patches( + run_login_side_effect=[fa.AuthError("unrelated")] + ) + [ + mock.patch.object(fa, "_can_prompt_for_revoke", return_value=True), + mock.patch.object(fa, "_prompt_revoke_and_wait", return_value=True), + ] + self._start(patches) + with self.assertRaises(fa.AuthError): + fa._cmd_login([]) + if __name__ == "__main__": unittest.main()