Route revoke through Connection#post_form, restore typed error mapping#29
Open
ahorovit wants to merge 1 commit into
Open
Route revoke through Connection#post_form, restore typed error mapping#29ahorovit wants to merge 1 commit into
ahorovit wants to merge 1 commit into
Conversation
The previous revoke_token implementation bypassed both @connection and handle_response by calling Net::HTTP.post_form directly. That worked for the wire format but had two costs: 1. Two HTTP code paths in one gem. get_token / refresh_token / every Resource method go through @connection (Faraday). revoke_token went through Net::HTTP. Two libraries, two error-handling surfaces, two places to patch if the gem ever changes HTTP behavior globally. 2. Lost typed error mapping. handle_response raises ConvertKit::UnauthorizedClientError, InvalidRequestError, etc. based on the OAuth 2.0 error code in the response body. The Net::HTTP path raised a flat ConvertKit::OauthError for every non-2xx, with the error code only available as text in the message string. Callers wanting to react differently to unauthorized_client vs invalid_request can't. Add Connection#post_form — a small, well-scoped method that POSTs with application/x-www-form-urlencoded instead of the connection's default application/json. Then revoke_token routes through @connection.post_form + handle_response, restoring the typed error mapping and unifying the HTTP path with the rest of the gem. OAuth 2.0 actually requires form-encoded for /oauth/token too (RFC 6749 §3.2); Kit currently tolerates JSON there. If that ever changes, post_form is reusable without further surgery.
roqmarcelo
approved these changes
Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Route
OAuth#revoke_tokenthrough@connectionandhandle_responseinstead of callingNet::HTTP.post_formdirectly. AddConnection#post_formto make form-encoded posts a first-class operation on the gem's existing HTTP pipeline. Wire format is identical to what's running onmaintoday — only the in-gem plumbing changes.Problem
PR #28 fixed the silent-no-op revoke bug by sending
application/x-www-form-urlencodedinstead ofapplication/jsonper RFC 7009. The bytes leaving the process are correct, but the implementation reached forNet::HTTP.post_formto enforce that Content-Type and bypassed the gem'sConnectionclass entirely. Two costs:Two HTTP code paths in one gem. Every other method —
get_token,refresh_token, everyResources::*method — goes through@connection(Faraday). Onlyrevoke_tokenwent through stdlibNet::HTTP. Two HTTP libraries, two response-shape contracts, two places to patch if the gem's HTTP behavior ever changes globally.Lost typed error mapping. The gem's
handle_responsealready maps OAuth 2.0 error codes to typed Ruby exception classes:unauthorized_clientConvertKit::UnauthorizedClientErrorinvalid_requestConvertKit::InvalidRequestErroraccess_deniedConvertKit::AccessDeniedErrorunsupported_response_typeConvertKit::UnsupportedResponseTypeErrorinvalid_scopeConvertKit::InvalidScopeErrorConvertKit::OauthErrorThe
Net::HTTPpath bypassed all of that and raised a flatConvertKit::OauthErrorfor every non-2xx, with the OAuth error code only readable as text inside the message string. Callers wanting to react differently tounauthorized_client(prompt user to reconnect) vsinvalid_request(transient retry) couldn't.This isn't theoretical. A staging4 disconnect attempt this week returned
unauthorized_clientfrom Kit. Speckel's caller code rescuesStandardErrortoday, but the next iteration of that caller will want to distinguish auth-failure from other failures — and right now it can't.Solution
Connection#post_formA new public method on
Connectionthat POSTs withapplication/x-www-form-urlencodedinstead of the connection's defaultapplication/json:Six lines. The response object is a Faraday response — same shape as every other method on
Connectionreturns — soprocess_response's JSON-body parsing and HTTP-status error mapping apply unchanged. Downstreamhandle_responseworks against it without modification.OAuth#revoke_tokenDrops
require 'net/http', drops the inline status-code check, drops the bespoke error message. Matches the shape ofget_tokenandrefresh_token:When Kit returns 2xx with an empty body (the RFC 7009 success path),
handle_responsereturns the raw response andrevoke_tokenreturnstrue. When Kit returns a 4xx with an OAuth error body,handle_responseraises the matching typed exception —ConvertKit::UnauthorizedClientErrorfor theunauthorized_clientcase we've already seen in production.Wire format
Unchanged from what
mainproduces today:URI.encode_www_formproduces the same byte sequenceNet::HTTP.post_formdoes internally — both are calling into the sameURIstdlib code under the covers. If the wire format onmainsatisfies Kit, this PR's does too.Design Decisions
post_formas a public method instead of an option to existingpostpostis dynamically defined inside anHTTP_METHODS.eachblock that doesn't naturally take an options hash. A second public method is less invasive than restructuring the dispatch loop, andpost_formis a meaningfully different operation worth its own name.Net::HTTPout of the gem entirelyrevoke_tokenback on Faraday means future maintainers think about one HTTP library, not two.handle_responserather than parse errors inlinehandle_responseis the gem's existing convention for the OAuth endpoint family (get_token,refresh_tokenalready use it). Restoringrevoke_tokento that pattern means the OAuth class has one error-handling shape, not two.Blast Radius
Gem-internal change with a public method signature unchanged.
ConvertKit::OAuth.new(id, secret).revoke_token(token)works exactly the same way for callers.Behavior change on failure paths: callers that were catching
ConvertKit::OauthErrorfor revoke errors will now catch typed exceptions instead. In practice the only caller is Speckel'sConvertKitService#revoke_token_for_provider, which usesrescue StandardError => error— all the typed exceptions areStandardErrorsubclasses, so existing logic still works. The improvement is opt-in: callers who want to distinguish error types can now do so.No change to the success path. Same return value (
true), same wire format.Rollback Plan
Revert via GitHub's Revert button. Restores the
Net::HTTP.post_formpath from PR #28 — a known steady state that's been running since PR #28 merged.Test plan
Verifiable by agent before merging
.ruby-version): 141 examples, 0 failures, 99.51% coverage. The newConnection#post_formspec brings the count up from 140.Connection#post_formspec asserts: correct path, body equalsURI.encode_www_form(params), headerContent-Type: application/x-www-form-urlencoded.OAuth#revoke_tokensuccess spec asserts:connection.post_formcalled with the exact params hash includingtoken_type_hint: 'access_token', returnstrue.OAuth#revoke_tokenfailure spec asserts: when Kit returns{"error":"unauthorized_client", ...}, the method raisesConvertKit::UnauthorizedClientErrorwith theerror_description. Directly motivated by the staging4 production observation.Cannot be verified before merge
main, so ifmain's wire format is right, this one is too — but only a real round-trip proves Kit's side processes it.