diff --git a/server/src/crate_server/context.gleam b/server/src/crate_server/context.gleam index 8c9a8e0..5297c7b 100644 --- a/server/src/crate_server/context.gleam +++ b/server/src/crate_server/context.gleam @@ -82,8 +82,18 @@ pub fn optional_field( /// XRPC error body: a PascalCase `error` name derived from the status plus the /// human-readable `message`. pub fn error_json(status: Int, message: String) -> Response { + named_error_json(status, error_name(status), message) +} + +/// `error_json` for a failure a client must branch on: the status alone does +/// not say which of several conditions produced it. +pub fn named_error_json( + status: Int, + error: String, + message: String, +) -> Response { json.object([ - #("error", json.string(error_name(status))), + #("error", json.string(error)), #("message", json.string(message)), ]) |> json.to_string diff --git a/server/src/crate_server/handlers/graph.gleam b/server/src/crate_server/handlers/graph.gleam index 77e2780..b0b5ada 100644 --- a/server/src/crate_server/handlers/graph.gleam +++ b/server/src/crate_server/handlers/graph.gleam @@ -8,7 +8,7 @@ import atproto_core/xrpc as core_xrpc import crate/gen/graph/follow.{type GraphFollow} import crate/storage.{type StoredItem} import crate_server/context.{ - type Context, error_json, require_session, with_pds_client, + type Context, error_json, named_error_json, require_session, with_pds_client, } import crate_server/follow_index import crate_server/graph_follows @@ -72,16 +72,14 @@ fn follow_response(uri: String) -> Response { } /// Existing sessions may not have the graph.follow scope that current OAuth -/// requests ask for. Preserve the authorization signal so the browser can -/// offer an explicit reauthorization instead of hiding it as an upstream -/// outage. +/// requests ask for. Only that condition is reported as a 403: a suspended or +/// taken-down account is also a PDS 403, and offering it reauthorization is a +/// loop the user cannot leave. fn pds_error(error: XrpcError, fallback: String) -> Response { case error { core_xrpc.BadStatus(_, Some(code), _, _) if code == "InsufficientScope" || code == "insufficient_scope" -> insufficient_scope_response() - core_xrpc.BadStatus(status, _, _, _) if status == 403 -> - insufficient_scope_response() core_xrpc.BadStatus(status, _, _, _) if status == 401 -> error_json(401, "your session expired while writing the follow record") _ -> error_json(502, fallback) @@ -89,8 +87,9 @@ fn pds_error(error: XrpcError, fallback: String) -> Response { } fn insufficient_scope_response() -> Response { - error_json( + named_error_json( 403, + "InsufficientScope", "your session needs permission to follow collectors; sign in again to approve it", ) } diff --git a/server/test/graph_handler_test.gleam b/server/test/graph_handler_test.gleam index f3de9e6..2b71c63 100644 --- a/server/test/graph_handler_test.gleam +++ b/server/test/graph_handler_test.gleam @@ -96,6 +96,21 @@ fn network_client(list_body: String, create_body: String) -> xrpc.Client { }) } +fn forbidden_client(error_code: String) -> xrpc.Client { + xrpc.Client(send: fn(req) { + case req.path { + "/xrpc/com.atproto.repo.listRecords" -> ok_response(list_records_body([])) + "/xrpc/com.atproto.repo.putRecord" -> + Ok(response.Response( + 403, + [], + bit_array.from_string("{\"error\":\"" <> error_code <> "\"}"), + )) + _ -> panic as { "unexpected path: " <> req.path } + } + }) +} + fn missing_scope_client() -> xrpc.Client { xrpc.Client(send: fn(req) { case req.path { @@ -272,13 +287,31 @@ pub fn follow_scope_failure_is_exposed_as_a_reauthorization_signal_test() { ) assert resp.status == 403 let body = simulate.read_body(resp) - assert support.field_string(body, ["error"]) == Ok("Forbidden") + assert support.field_string(body, ["error"]) == Ok("InsufficientScope") assert support.field_string(body, ["message"]) == Ok( "your session needs permission to follow collectors; sign in again to approve it", ) } +/// A suspended or taken-down account is a PDS 403 too. Answering it with the +/// reauthorization prompt sends the user round a loop that cannot help them. +pub fn a_non_scope_pds_403_is_an_upstream_failure_not_a_scope_prompt_test() { + ["AccountTakedown", "AccountDeactivated", "Forbidden"] + |> list.each(fn(code) { + let resp = + call( + graph.follow, + "graph.followUser", + forbidden_client(code), + support.fresh_follow_index(), + ) + assert resp.status == 502 + assert support.field_string(simulate.read_body(resp), ["error"]) + == Ok("UpstreamFailure") + }) +} + pub fn follow_mirrors_the_written_record_into_the_index_test() { let follows = seeded_index([]) assert call(graph.follow, "graph.followUser", no_list_client(), follows).status diff --git a/web/src/crate_web/update/common.gleam b/web/src/crate_web/update/common.gleam index dd0fa64..5457f4a 100644 --- a/web/src/crate_web/update/common.gleam +++ b/web/src/crate_web/update/common.gleam @@ -49,15 +49,16 @@ pub fn write_error( } } -/// Follow writes use a distinct 403 response when an existing OAuth session -/// lacks the graph.follow scope. Other writes keep the generic error path. +/// Follow writes answer `InsufficientScope` when an existing OAuth session +/// lacks the graph.follow scope. Every other 403 (a suspended or taken-down +/// account, say) takes the generic path: reauthorizing would not fix it. pub fn follow_write_error( model: Model, error: msg.ApiError, fallback: String, ) -> #(Model, Effect(Msg)) { case error { - xrpc.BadStatus(status: 403, ..) -> #( + xrpc.BadStatus(status: 403, error: Some("InsufficientScope"), ..) -> #( Model( ..model, busy: False, diff --git a/web/test/connections_test.gleam b/web/test/connections_test.gleam index f67f751..2699940 100644 --- a/web/test/connections_test.gleam +++ b/web/test/connections_test.gleam @@ -214,6 +214,34 @@ pub fn stale_follow_completion_after_same_route_reload_is_ignored_test() { assert after_stale.connection_pending == None } +/// A suspended or taken-down account is a 403 too, and reauthorizing does +/// nothing for it: only the scope code earns the reauthorization prompt. +pub fn a_non_scope_403_does_not_offer_reauthorization_test() { + ["AccountTakedown", "Forbidden", "UpstreamFailure"] + |> list.each(fn(code) { + let #(pending, _) = + update( + loaded_follow_state(False), + ToggleConnectionFollow("did:bob", False), + ) + let assert Some(request) = pending.connection_pending + let #(updated, _) = + update( + pending, + GotConnectionFollow( + request, + Error(xrpc.BadStatus(403, Some(code), None, "")), + ), + ) + assert updated.notice != None + assert case updated.notice { + Some(ReauthorizeNotice(_)) -> True + _ -> False + } + == False + }) +} + pub fn missing_follow_permission_offers_reauthorization_in_connections_test() { let #(pending, effect) = update(loaded_follow_state(False), ToggleConnectionFollow("did:bob", False))