From 73d7f70abd1a1529b56df679136e46661884acd1 Mon Sep 17 00:00:00 2001 From: Jankees van Woezik Date: Sun, 30 Aug 2026 09:12:10 +0200 Subject: [PATCH 01/10] Put the publisher's standing on every listing entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A native browser renders a trust badge on a card, and until now the only place that said whether a namespace had been claimed was the publisher response — so a grid had to fetch a publisher per card to find out, or imply endorsement it could not back up. `publisher_claimed` and `publisher_verified` ride along on the shared plugin partial, which the directory, a publisher's plugin list and a detail response all render. Flat keys rather than a nested object: `publisher` is already a string in this shape and clients parse it as one. Adding fields within a schema_version is what the contract allows. Development also stops hardcoding its own base URL. Absolute URLs in the browse API are built from `registry_base_url`, and a client that opens a plugin page reads them literally — so running the server on any port but 3000 sent people somewhere that was not listening. --- app/views/plugins/_plugin.json.jbuilder | 8 ++++++++ config/environments/development.rb | 5 ++++- docs/browse-api.md | 8 ++++++++ test/integration/browse_api_test.rb | 23 +++++++++++++++++++++++ 4 files changed, 43 insertions(+), 1 deletion(-) diff --git a/app/views/plugins/_plugin.json.jbuilder b/app/views/plugins/_plugin.json.jbuilder index d0df6c6..13959b7 100644 --- a/app/views/plugins/_plugin.json.jbuilder +++ b/app/views/plugins/_plugin.json.jbuilder @@ -4,6 +4,14 @@ # comments) is layered on by plugins/show. json.id plugin.manifest_id json.publisher plugin.publisher.name +# The namespace's standing, not the plugin's. `claimed` false means the +# listing was seeded from the legacy marketplace and nobody has proven control +# of the source repo — a client that shows a trust badge needs this on the +# listing, not only on the detail response, or it has to fetch a publisher per +# card to find out. Flat keys rather than a nested object: `publisher` is a +# string in this shape already and clients parse it as one. +json.publisher_claimed plugin.publisher.claimed? +json.publisher_verified plugin.publisher.verified? json.name plugin.name json.full_name plugin.full_name json.summary plugin.summary diff --git a/config/environments/development.rb b/config/environments/development.rb index a52c1ee..fd3328d 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -3,7 +3,10 @@ Rails.application.configure do config.x.skip_first_release_gate = true config.x.publish_hold = 0 - config.x.registry_base_url = "http://localhost:3000" + # Absolute URLs in the browse API and the data plane are built from this, so + # it has to follow the port the server is actually on. A native client that + # opens a plugin page reads it literally. + config.x.registry_base_url = ENV.fetch("REGISTRY_BASE_URL", "http://localhost:3000") # Settings specified here will take precedence over those in config/application.rb. # Make code changes take effect immediately without server restart. diff --git a/docs/browse-api.md b/docs/browse-api.md index ca82109..fdd283f 100644 --- a/docs/browse-api.md +++ b/docs/browse-api.md @@ -128,6 +128,8 @@ One plugin, with everything the web page renders. "plugin": { "id": "acme.weather", "publisher": "acme", + "publisher_claimed": true, + "publisher_verified": false, "name": "weather", "full_name": "acme/weather", "summary": "Forecast in the bar", @@ -171,6 +173,12 @@ One plugin, with everything the web page renders. - `install_command` is `null` when the plugin is not installable. Use `installable` to decide whether to offer an install button — a security hold or a plugin with nothing through review must not present one. +- `publisher_claimed` / `publisher_verified` are the namespace's standing, not + the plugin's, and they are on every plugin entry — including in the + directory listing, so a grid can render a trust badge without fetching a + publisher per card. `publisher_claimed: false` means the listing was seeded + from the legacy marketplace and no author has proven control of the source + repo; say so rather than implying the namespace is endorsed. - `viewer` appears only for an authenticated session. Anonymous clients never see it, and those responses are the publicly cacheable ones. - `first_published_at` / `last_published_at` are populated on directory diff --git a/test/integration/browse_api_test.rb b/test/integration/browse_api_test.rb index e74278e..cfdb665 100644 --- a/test/integration/browse_api_test.rb +++ b/test/integration/browse_api_test.rb @@ -32,6 +32,24 @@ def seed_filler(count) # --- directory ----------------------------------------------------------- + # A namespace seeded from the legacy marketplace has nobody behind it yet, + # and a client must be able to say so instead of implying endorsement. + test "an unclaimed publisher is said so on the listing entry" do + @acme.update!(claimed: false) + + get directory_json_path + assert_response :success + refute body["plugins"].sole["publisher_claimed"] + end + + test "a verified publisher is said so on the listing entry" do + @acme.update!(verified: true) + + get directory_json_path + assert_response :success + assert body["plugins"].sole["publisher_verified"] + end + test "directory JSON lists plugins with the browse vocabulary attached" do get directory_json_path assert_response :success @@ -47,6 +65,11 @@ def seed_filler(count) assert_equal "omarchy plugin add acme/weather", entry["install_command"] assert_equal "http://registry.test/plugins/acme/weather", entry["url"] + # The namespace's standing travels with every entry, so a grid can render + # a trust badge without fetching a publisher per card. + assert entry["publisher_claimed"] + refute entry["publisher_verified"] + # Facets a client would otherwise have to hardcode assert_includes body["taxonomy"]["sorts"], "trending" assert_includes body["taxonomy"]["tags"], "weather" From e31e5a4c6b362778ad177578084c0945d8f1f467 Mon Sep 17 00:00:00 2001 From: Jankees van Woezik Date: Sun, 30 Aug 2026 09:20:17 +0200 Subject: [PATCH 02/10] Let an app sign in without handing it the keys to publish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The desktop plugin browser needs to know who you are and to post a rating or a comment. The only token this registry could mint was push-only, minted behind a freshly proved second factor and expiring in seven days — the right shape for `omarchy plugin publish` and the wrong one for an app you leave open. api_tokens gain a `kind`. `publish` is everything that exists today, unchanged. `client` can read as you, rate and comment, and can never publish: the check is at the endpoint, so a publish token posted to a client endpoint is a 403 and so is the reverse. Client tokens live 30 days and get their own budget, because a handful of laptops must not lock someone out of shipping a release. Approving one does not demand a second factor. The browser session doing the approving can already rate and comment without proving one, so gating the app would be theatre — and it would lock every account without MFA out of the app entirely. Publishing keeps the bar; it ships code to other people's machines. The sensitive-change cooldown still applies to both. The scope is decided when the authorization row is created, so nothing the approving browser sends can turn a client request into a publish token, and an unrecognised scope falls back to publish rather than to the weaker one. The device response also carries verification_uri_complete, so a desktop app can open a page the user only has to approve instead of making them retype a code the app already knows. The new /api/v1 surface is small on purpose: `me` and `session` for identity, and one social payload — rating summary, your own rating, and the thread — that every write answers with, so the client applies one response instead of stitching two together. Commenting shares the web form's five-an-hour budget; a second door onto the same table must not be the cheap way around the first one's limit. A plugin that has never been public 404s exactly as a name that never existed does. Suspension now kills live API tokens the way it already killed browser sessions, said as forbidden rather than unauthorized — telling a suspended publisher their token expired only sends them to mint another. --- README.md | 8 +- app/controllers/api/base_controller.rb | 33 ++- app/controllers/api/v1/comments_controller.rb | 36 +++ app/controllers/api/v1/device_controller.rb | 16 +- app/controllers/api/v1/me_controller.rb | 42 +++ app/controllers/api/v1/plugins_controller.rb | 20 ++ app/controllers/api/v1/ratings_controller.rb | 32 +++ app/controllers/api/v1/versions_controller.rb | 2 +- .../concerns/api/v1/plugin_scoped.rb | 62 ++++ app/controllers/device_controller.rb | 32 ++- app/models/api_token.rb | 49 +++- app/models/current.rb | 7 +- app/models/device_authorization.rb | 31 +- app/views/device/show.html.erb | 26 +- config/routes.rb | 14 + .../20260830090001_add_kind_to_api_tokens.rb | 14 + db/schema.rb | 4 +- docs/client-api.md | 186 ++++++++++++ test/integration/client_api_test.rb | 269 ++++++++++++++++++ 19 files changed, 845 insertions(+), 38 deletions(-) create mode 100644 app/controllers/api/v1/comments_controller.rb create mode 100644 app/controllers/api/v1/me_controller.rb create mode 100644 app/controllers/api/v1/plugins_controller.rb create mode 100644 app/controllers/api/v1/ratings_controller.rb create mode 100644 app/controllers/concerns/api/v1/plugin_scoped.rb create mode 100644 db/migrate/20260830090001_add_kind_to_api_tokens.rb create mode 100644 docs/client-api.md create mode 100644 test/integration/client_api_test.rb diff --git a/README.md b/README.md index 320b09d..dc6c866 100644 --- a/README.md +++ b/README.md @@ -41,9 +41,11 @@ pipeline with deterministic scanning, capability fingerprints + delta holds, escalate-only AI review hook, publish hold window, Ed25519-signed index + kill list, device-flow CLI login, OIDC trusted publishing with provenance, passkeys, community (ratings/comments/views/reports + moderation), seeding + repo-proof -claims, the admin console, and a JSON browse API for a native in-desktop -plugin browser (docs/browse-api.md — unsigned browse data, never an install -path). +claims, the admin console, and the two JSON APIs a native in-desktop plugin +browser reads and writes: `docs/browse-api.md` (anonymous, cacheable browse +data — never an install path) and `docs/client-api.md` (device-flow sign-in +and the ratings and comments a signed-in app posts, on a token that can never +publish). **Not yet done, and required before launch**: the Quattro-side client (`omarchy plugin add/update/publish`, signature + freshness verification, diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index 7ef392e..ad56fa4 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -16,12 +16,39 @@ def respond_to_missing?(name, include_private = false) = Rails.cache.respond_to? private - def authenticate_api_token! + # Every authenticated endpoint names the kind of token it accepts. A + # publish token cannot post a comment and a client token cannot publish, + # and neither can be mistaken for the other by forgetting to look. + def authenticate_api_token!(kind:) raw = request.authorization.to_s[/\ABearer (.+)\z/, 1] - @current_token = ApiToken.authenticate(raw) - render json: { error: "invalid or expired token" }, status: :unauthorized unless @current_token + token = ApiToken.authenticate(raw) + + if token.nil? + return render json: { error: "invalid or expired token" }, status: :unauthorized + end + unless token.kind == kind.to_s + return render json: { error: "this token is not a #{kind} token" }, status: :forbidden + end + # Suspension kills live credentials, not just future sign-ins — the same + # rule the cookie session follows. Said as forbidden rather than + # unauthorized: the token is fine, the account is not, and telling a + # suspended publisher their token expired sends them to mint another. + if token.user.suspended_at.present? + return render json: { error: "account is suspended" }, status: :forbidden + end + + @current_token = token + # Domain code reads Current.user — comment authorship, the publisher + # badge, audit attribution. Setting it here means an API request and a + # browser request are the same request as far as the models are + # concerned. CurrentAttributes resets between requests. + Current.api_user = token.user end + def authenticate_publish_token! = authenticate_api_token!(kind: :publish) + def authenticate_client_token! = authenticate_api_token!(kind: :client) + attr_reader :current_token + def current_user = @current_token&.user end end diff --git a/app/controllers/api/v1/comments_controller.rb b/app/controllers/api/v1/comments_controller.rb new file mode 100644 index 0000000..1b7ed08 --- /dev/null +++ b/app/controllers/api/v1/comments_controller.rb @@ -0,0 +1,36 @@ +module Api + module V1 + class CommentsController < BaseController + include PluginScoped + + before_action :authenticate_client_token! + before_action :load_plugin!, only: :create + after_action { response.headers["Cache-Control"] = "no-store" } + + # The same budget the web form gets. A second door onto the same table + # must not be the cheap way around the first one's limit. + rate_limit to: 5, within: 1.hour, only: :create, store: RATE_LIMIT_STORE, + by: -> { request.authorization.to_s }, + with: -> { render json: { error: "slow down — try again in a bit" }, status: :too_many_requests } + + def create + comment = @plugin.comments.new(user: current_user, body: params[:body]) + if comment.save + render json: social_payload, status: :created + else + render json: { error: comment.errors.full_messages.join("; ") }, status: :unprocessable_entity + end + end + + # Authors delete their own comments. Hiding someone else's is moderation + # and stays in the MFA-gated admin controllers, where it leaves an audit + # trail — scoping to the user's own comments is what keeps it that way. + def destroy + comment = current_user.comments.find(params[:id]) + plugin = comment.plugin + comment.destroy! + render json: social_payload(plugin) + end + end + end +end diff --git a/app/controllers/api/v1/device_controller.rb b/app/controllers/api/v1/device_controller.rb index 68698e8..1bd0dbe 100644 --- a/app/controllers/api/v1/device_controller.rb +++ b/app/controllers/api/v1/device_controller.rb @@ -11,16 +11,25 @@ class DeviceController < BaseController rate_limit to: 240, within: 15.minutes, only: :token, store: RATE_LIMIT_STORE, with: -> { render json: { error: "slow_down" }, status: :too_many_requests } - # POST /api/v1/device/code — CLI starts the flow, optionally naming the - # publisher/plugin it wants so the approval page can display the scope - # instead of making the human re-type it + # POST /api/v1/device/code — a device starts the flow, optionally naming + # the publisher/plugin it wants so the approval page can display the + # scope instead of making the human re-type it. + # + # `scope=client` asks for a token that can rate and comment but never + # publish; anything else, including nothing, asks for a publish token. + # verification_uri_complete carries the code in the URL so a desktop app + # can open a page the user only has to approve — the bare + # verification_uri stays for a terminal that can only print one. def code authorization = DeviceAuthorization.start!( + token_kind: params[:scope] == "client" ? :client : :publish, requested_publisher: params[:publisher], requested_plugin: params[:plugin]) render json: { device_code: authorization.plaintext_device_code, user_code: authorization.user_code, verification_uri: "#{DataPlane.base_url}/device", + verification_uri_complete: "#{DataPlane.base_url}/device?code=#{authorization.user_code}", + scope: authorization.token_kind, expires_in: DeviceAuthorization::EXPIRATION.to_i, interval: DeviceAuthorization::POLL_INTERVAL }, status: :created @@ -48,6 +57,7 @@ def token render json: { token: plaintext, token_type: "bearer", + kind: authorization.api_token&.kind, scope: authorization.api_token&.scope_label, expires_at: authorization.api_token&.expires_at&.utc&.iso8601 } diff --git a/app/controllers/api/v1/me_controller.rb b/app/controllers/api/v1/me_controller.rb new file mode 100644 index 0000000..9d05860 --- /dev/null +++ b/app/controllers/api/v1/me_controller.rb @@ -0,0 +1,42 @@ +module Api + module V1 + # Who the signed-in app is talking as, and how it signs out. + class MeController < BaseController + before_action :authenticate_client_token! + after_action { response.headers["Cache-Control"] = "no-store" } + + # A client calls this right after the device flow, and again on every + # launch: it is how the app finds out its stored token is still good + # without having to guess from a failed write. + def show + render json: { + user: { + name: current_user.name, + email: current_user.email_address, + admin: current_user.admin? + }, + # The namespaces this account publishes under. The browser uses the + # personal one as the handle behind "My plugins", which is a much + # better answer than the one it guesses from installed plugin ids. + publishers: current_user.publishers.map do |publisher| + { name: publisher.name, kind: publisher.kind, + personal: publisher.personal?, verified: publisher.verified? } + end, + token: { + hint: current_token.token_hint, + expires_at: current_token.expires_at.utc.iso8601, + scope: current_token.scope_label + } + } + end + + # Signing out in the app revokes the token rather than only forgetting + # it. A token the client has thrown away but the registry still honours + # is exactly the credential nobody notices leaking. + def destroy + current_token.revoke! + head :no_content + end + end + end +end diff --git a/app/controllers/api/v1/plugins_controller.rb b/app/controllers/api/v1/plugins_controller.rb new file mode 100644 index 0000000..946eb9f --- /dev/null +++ b/app/controllers/api/v1/plugins_controller.rb @@ -0,0 +1,20 @@ +module Api + module V1 + # The viewer's slice of a plugin: the comment thread, and where this + # account stands on it. + # + # The public read of a plugin is the browse API (/plugins/:publisher/:name + # .json), which is anonymous and cacheable. This is the part that cannot + # be: it depends on who is asking, so it lives behind a token and is + # answered no-store. + class PluginsController < BaseController + include PluginScoped + + before_action :authenticate_client_token! + before_action :load_plugin! + after_action { response.headers["Cache-Control"] = "no-store" } + + def show = render json: social_payload + end + end +end diff --git a/app/controllers/api/v1/ratings_controller.rb b/app/controllers/api/v1/ratings_controller.rb new file mode 100644 index 0000000..8317b11 --- /dev/null +++ b/app/controllers/api/v1/ratings_controller.rb @@ -0,0 +1,32 @@ +module Api + module V1 + class RatingsController < BaseController + include PluginScoped + + before_action :authenticate_client_token! + before_action :load_plugin! + after_action { response.headers["Cache-Control"] = "no-store" } + + # Idempotent: rating a plugin you have already rated moves your rating + # rather than failing on the one-per-user constraint, which is what a + # star control does when you click a different star. + def update + value = params[:value].to_i + unless (1..5).cover?(value) + return render json: { error: "value must be between 1 and 5" }, status: :unprocessable_entity + end + + rating = @plugin.ratings.find_or_initialize_by(user: current_user) + rating.update!(value: value) + render json: social_payload + end + + # Clearing a rating is not the same as rating something one star, so it + # gets its own verb rather than a magic value. + def destroy + @plugin.ratings.find_by(user: current_user)&.destroy! + render json: social_payload + end + end + end +end diff --git a/app/controllers/api/v1/versions_controller.rb b/app/controllers/api/v1/versions_controller.rb index 2572959..e7e7e67 100644 --- a/app/controllers/api/v1/versions_controller.rb +++ b/app/controllers/api/v1/versions_controller.rb @@ -9,7 +9,7 @@ class VersionsController < BaseController rate_limit to: 30, within: 15.minutes, only: :create, store: RATE_LIMIT_STORE, with: -> { render json: { error: "slow_down" }, status: :too_many_requests } - before_action :authenticate_api_token! + before_action :authenticate_publish_token! MAX_BODY_BYTES = Registry::TarballInspector::MAX_TARBALL_BYTES diff --git a/app/controllers/concerns/api/v1/plugin_scoped.rb b/app/controllers/concerns/api/v1/plugin_scoped.rb new file mode 100644 index 0000000..4bc53fa --- /dev/null +++ b/app/controllers/concerns/api/v1/plugin_scoped.rb @@ -0,0 +1,62 @@ +module Api + module V1 + # Loading a plugin by its "publisher/name" pair, and answering with the + # social state of it. Shared by every endpoint the desktop browser writes + # through, so a rating, a comment and a delete all hand back the same + # shape — the client applies one response and never has to stitch two + # together or refetch to find out what happened. + module PluginScoped + extend ActiveSupport::Concern + + # The same fifty the web page shows. A thread longer than this wants + # paging, not a bigger number. + COMMENT_LIMIT = 50 + + included do + rescue_from ActiveRecord::RecordNotFound do + render json: { error: "not found" }, status: :not_found + end + end + + private + + def load_plugin! + publisher = Publisher.find_by!(name: params[:publisher]) + @plugin = publisher.plugins.find_by!(name: params[:plugin]) + # A plugin that has never been public 404s exactly as a name that + # never existed does — the same rule the web page follows, so the API + # is not a way to enumerate what is still in review. + raise ActiveRecord::RecordNotFound unless @plugin.visible_to?(current_user) + end + + def social_payload(plugin = @plugin) + comments = plugin.comments.visible.includes(:user).order(created_at: :desc).limit(COMMENT_LIMIT) + # One query for the badge rather than a membership lookup per comment. + member_ids = plugin.publisher.memberships.accepted.pluck(:user_id).to_set + + { + plugin: plugin.manifest_id, + rating: { + average: plugin.average_rating, + count: plugin.ratings_count, + mine: plugin.ratings.find_by(user: current_user)&.value + }, + comments: comments.map do |comment| + { + id: comment.id, + body: comment.body, + created_at: comment.created_at.utc.iso8601, + # Whether this account can delete it. Authors delete their own; + # everything else is moderation. + mine: comment.user_id == current_user.id, + author: { + name: comment.user.name, + publisher_member: member_ids.include?(comment.user_id) + } + } + end + } + end + end + end +end diff --git a/app/controllers/device_controller.rb b/app/controllers/device_controller.rb index 295f160..76dd593 100644 --- a/app/controllers/device_controller.rb +++ b/app/controllers/device_controller.rb @@ -1,6 +1,15 @@ -# Browser side of the CLI device flow: enter the code, pick the scope, approve. +# Browser side of the device flow: enter the code, see what it grants, approve. +# +# Two devices come through here — `omarchy plugin publish` asking for a publish +# token, and the desktop plugin browser asking to sign in — and they are not +# held to the same bar. Minting something that can ship code needs a freshly +# proved second factor. A client token can only do what this very browser +# session can already do without one, so demanding a passkey before you may +# leave a comment would be theatre, and would lock everyone without MFA out of +# the app entirely. The sensitive-change cooldown still applies to both: it +# gates credential-shaped actions, and this is one. class DeviceController < ApplicationController - before_action :require_recent_second_factor, only: :approve + before_action :require_recent_second_factor, only: :approve, unless: :approving_client? before_action :require_no_sensitive_cooldown, only: :approve def show @@ -29,7 +38,22 @@ def approve return redirect_to dashboard_path, alert: e.record.errors.full_messages.join("; ") end AuditEvent.record!(actor: Current.user, action: "device.approve", subject: authorization, - metadata: { scope: token.scope_label }) - redirect_to dashboard_path, notice: "Approved — your terminal has a publish token for your account. It expires in 7 days." + metadata: { kind: token.kind, scope: token.scope_label }) + + notice = if token.client? + "Signed in — the plugin browser is connected to your account for 30 days." + else + "Approved — your terminal has a publish token for your account. It expires in 7 days." + end + redirect_to dashboard_path, notice: notice + end + + private + + # Reads the pending authorization rather than anything the request says, so + # the weaker gate can only ever be reached by a row that was created asking + # for a client token. + def approving_client? + DeviceAuthorization.find_by_user_code(params[:code])&.for_client? || false end end diff --git a/app/models/api_token.rb b/app/models/api_token.rb index da4c471..dc7eca1 100644 --- a/app/models/api_token.rb +++ b/app/models/api_token.rb @@ -1,14 +1,28 @@ -# Short-lived, push-only. Account-wide by default (like RubyGems/npm): the -# token can publish to any namespace its user is a member of, which the publish -# path enforces via membership regardless of the token. A non-null publisher -# and/or plugin_name NARROWS the scope — trusted publishing (OIDC) mints -# per-plugin tokens for CI. The plaintext token exists only at mint time; we -# store a SHA-256 digest. No long-lived classic tokens, ever. +# Short-lived and single-purpose. The plaintext token exists only at mint +# time; we store a SHA-256 digest. No long-lived classic tokens, ever. +# +# `kind` is the hard boundary between the two things a token can be, and it is +# checked at the endpoint rather than inferred from scope: +# +# publish — push-only, the device flow behind `omarchy plugin publish`. +# Account-wide by default (like RubyGems/npm): it can publish to any +# namespace its user is a member of, which the publish path enforces via +# membership regardless of the token. A non-null publisher and/or +# plugin_name NARROWS it — trusted publishing (OIDC) mints per-plugin +# tokens for CI. +# +# client — the desktop plugin browser. It can say who you are and post a +# rating or a comment, and it can never publish. It lives longer because +# it backs a signed-in app rather than one command, and it is worth much +# less if it leaks: everything it can do, it can do in the browser too. class ApiToken < ApplicationRecord DEFAULT_TTL = 7.days + CLIENT_TTL = 30.days MAX_TTL = 90.days PREFIX = "omp_" + enum :kind, { publish: 0, client: 1 } + belongs_to :user belongs_to :publisher, optional: true @@ -17,6 +31,10 @@ class ApiToken < ApplicationRecord attr_accessor :quota_exempt MAX_USABLE_PER_USER = 25 + # Client tokens are one per signed-in app, not one per publish, so they get + # their own budget. Sharing the publish quota would let a handful of laptops + # lock someone out of shipping a release. + MAX_CLIENTS_PER_USER = 10 validates :plugin_name, format: { with: NameRules::NAME_FORMAT }, length: { maximum: NameRules::MAX_LENGTH }, allow_nil: true @@ -30,10 +48,11 @@ class ApiToken < ApplicationRecord scope :usable, -> { where(revoked_at: nil).where(expires_at: Time.current..) } - def self.mint!(user:, publisher: nil, plugin_name: nil, ttl: DEFAULT_TTL, quota_exempt: false) + def self.mint!(user:, publisher: nil, plugin_name: nil, kind: :publish, ttl: nil, quota_exempt: false) + ttl ||= kind.to_s == "client" ? CLIENT_TTL : DEFAULT_TTL raw = PREFIX + SecureRandom.base58(30) token = create!( - user:, publisher:, plugin_name:, + user:, publisher:, plugin_name:, kind:, token_digest: digest(raw), token_hint: "#{raw.first(8)}…#{raw.last(4)}", expires_at: ttl.from_now, @@ -45,7 +64,7 @@ def self.mint!(user:, publisher: nil, plugin_name: nil, ttl: DEFAULT_TTL, quota_ def self.authenticate(raw) return nil if raw.blank? - usable.find_by(token_digest: digest(raw))&.tap { |t| t.touch(:last_used_at) } + usable.includes(:user).find_by(token_digest: digest(raw))&.tap { |t| t.touch(:last_used_at) } end def self.digest(raw) = Digest::SHA256.hexdigest(raw) @@ -67,6 +86,7 @@ def authorizes?(publisher_arg, plugin_name_arg) # Human-readable scope for tokens list / API responses. def scope_label + return "browse, rate and comment as you" if client? return "account (any of your namespaces)" if publisher_id.nil? plugin_name.nil? ? "#{publisher.name}/*" : "#{publisher.name}/#{plugin_name}" end @@ -79,9 +99,18 @@ def ttl_within_bounds def usable_quota return if quota_exempt + return unless user + + if client? + if user.api_tokens.usable.client.count >= MAX_CLIENTS_PER_USER + errors.add(:base, "too many signed-in apps — sign out of one first") + end + return + end + # Machine-minted (OIDC/provenance) tokens neither consume nor count toward # the user-managed quota — a CI burst must not lock a human out - if user && user.api_tokens.usable.where(provenance: nil).count >= MAX_USABLE_PER_USER + if user.api_tokens.usable.publish.where(provenance: nil).count >= MAX_USABLE_PER_USER errors.add(:base, "too many active tokens — revoke some first") end end diff --git a/app/models/current.rb b/app/models/current.rb index 2bef56d..7343dfd 100644 --- a/app/models/current.rb +++ b/app/models/current.rb @@ -1,4 +1,9 @@ class Current < ActiveSupport::CurrentAttributes attribute :session - delegate :user, to: :session, allow_nil: true + # Set directly by the token-authenticated API, which has no cookie session. + # The browser path leaves it nil and falls through to the session's user, so + # domain code can read Current.user without caring which door was used. + attribute :api_user + + def user = api_user || session&.user end diff --git a/app/models/device_authorization.rb b/app/models/device_authorization.rb index 7ba1dcf..fd0fb22 100644 --- a/app/models/device_authorization.rb +++ b/app/models/device_authorization.rb @@ -1,8 +1,13 @@ -# CLI login without ever typing credentials into a terminal (RFC 8628 shape): -# `omarchy plugin publish` requests a code pair, the user approves the 8-char -# user code in the browser (MFA'd session), and the CLI polls until it -# receives a freshly minted scoped token. The token plaintext is held -# encrypted only until the CLI claims it, then wiped. +# Sign-in without ever typing credentials into the client (RFC 8628 shape): +# the client requests a code pair, the user approves the 8-char user code in +# the browser, and the client polls until it receives a freshly minted token. +# The token plaintext is held encrypted only until the client claims it, then +# wiped. +# +# Two clients use this. `omarchy plugin publish` asks for a publish token from +# a terminal, against an MFA'd session. The desktop plugin browser asks for a +# client token, which can only rate and comment — see ApiToken#kind for why +# that one is not held to the same bar. class DeviceAuthorization < ApplicationRecord belongs_to :api_token, optional: true EXPIRATION = 15.minutes @@ -10,6 +15,10 @@ class DeviceAuthorization < ApplicationRecord POLL_INTERVAL = 5 # seconds, advisory for clients enum :status, { pending: 0, approved: 1, denied: 2, claimed: 3 } + # Which kind of token this authorization will mint. Named on the request so + # the approval page can say what is being handed over, and so a browser + # asking to comment can never come back holding a publish token. + enum :token_kind, { publish: 0, client: 1 }, prefix: :for belongs_to :user, optional: true belongs_to :publisher, optional: true @@ -22,12 +31,13 @@ class DeviceAuthorization < ApplicationRecord # page can show what the terminal wants instead of asking the human to # re-type it. They never grant anything: approval still binds to a namespace # the signed-in user is a member of, chosen in the browser. - def self.start!(requested_publisher: nil, requested_plugin: nil) + def self.start!(requested_publisher: nil, requested_plugin: nil, token_kind: :publish) raw = "omd_" + SecureRandom.base58(30) authorization = create!( device_code_digest: digest(raw), user_code: generate_user_code, expires_at: EXPIRATION.from_now, + token_kind: sanitize_kind(token_kind), requested_publisher_name: sanitize_hint(requested_publisher), requested_plugin_name: sanitize_hint(requested_plugin) ) @@ -35,6 +45,13 @@ def self.start!(requested_publisher: nil, requested_plugin: nil) authorization end + # An unrecognised scope falls back to publish rather than to the weaker one: + # a caller who asks for something we do not understand gets the flow that is + # gated hardest, not the one that is gated least. + def self.sanitize_kind(value) + token_kinds.key?(value.to_s) ? value.to_s : "publish" + end + def self.sanitize_hint(value) hint = value.to_s.downcase.strip hint.match?(NameRules::NAME_FORMAT) ? hint : nil @@ -61,7 +78,7 @@ def self.normalize_user_code(code) # belongs to (membership is enforced at publish time). Passing a publisher # and/or plugin_name narrows it — kept for a future "tighter scope" UI. def approve!(user:, publisher: nil, plugin_name: nil) - token = ApiToken.mint!(user:, publisher:, plugin_name:) + token = ApiToken.mint!(user:, publisher:, plugin_name:, kind: token_kind) # The EXACT minted token is referenced — polling must report this token's # expiry, not whichever same-scope token happens to be newest update!(status: :approved, user:, publisher:, plugin_name:, api_token: token, diff --git a/app/views/device/show.html.erb b/app/views/device/show.html.erb index ea52361..6efad94 100644 --- a/app/views/device/show.html.erb +++ b/app/views/device/show.html.erb @@ -1,7 +1,19 @@ <% content_for :title, "Device sign-in — Omarchy Plugins" %>
-

CLI sign-in

- <% if @authorization %> + <% if @authorization&.for_client? %> +

App sign-in

+

Sign in to the plugin browser?

+

An app holding code <%= @authorization.user_code %> wants to + sign in as you. Only approve if you just clicked Sign in in the Omarchy plugin browser + yourself and the code above matches the one it is showing.

+
+ +

Reading the directory as you, and posting ratings and comments under your name. + It cannot publish, yank, change owners, or touch account settings, and it + expires in 30 days. Sign out in the app — or revoke it here — to end it sooner.

+
+ <% elsif @authorization %> +

CLI sign-in

Approve this terminal?

A device holding code <%= @authorization.user_code %> wants a publish token for your account. Only approve if you just ran @@ -17,15 +29,19 @@ It's push-only — it can't yank, change owners, or touch account settings — expires in 7 days, and every version still goes through review before it can go live.

+ <% end %> + + <% if @authorization %> <%= form_with url: approve_device_path do |form| %> <% end %> <% else %> -

Enter the code from your terminal.

-

omarchy plugin publish shows an 8-character code. Type it here to connect - that terminal to your account.

+

Device sign-in

+

Enter the code from your device.

+

omarchy plugin publish and the Omarchy plugin browser each show an 8-character + code. Type it here to connect that device to your account.

<%= form_with url: device_path, method: :get do |form| %>
diff --git a/config/routes.rb b/config/routes.rb index 1ff50cf..28071d8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -111,6 +111,20 @@ post "device/code", to: "device#code" post "device/token", to: "device#token" post "trusted/exchange", to: "trusted#exchange" + + # --- Client API --- + # What a signed-in desktop browser writes through. Reading a plugin is + # the anonymous, cacheable browse API; this is everything that depends + # on who is asking. Client tokens only — see ApiToken#kind. + get "me", to: "me#show" + delete "session", to: "me#destroy" + scope "plugins/:publisher/:plugin", constraints: { publisher: %r{[^/]+}, plugin: %r{[^/]+} } do + get "/", to: "plugins#show", as: :client_plugin + put "rating", to: "ratings#update", as: :client_plugin_rating + delete "rating", to: "ratings#destroy" + post "comments", to: "comments#create", as: :client_plugin_comments + end + delete "comments/:id", to: "comments#destroy", as: :client_comment end end diff --git a/db/migrate/20260830090001_add_kind_to_api_tokens.rb b/db/migrate/20260830090001_add_kind_to_api_tokens.rb new file mode 100644 index 0000000..0c669e2 --- /dev/null +++ b/db/migrate/20260830090001_add_kind_to_api_tokens.rb @@ -0,0 +1,14 @@ +# A token that browses is not a token that publishes. +# +# Every api_token so far has been push-only, minted through the device flow +# for `omarchy plugin publish`. The desktop plugin browser needs a token too, +# but only to say who you are and to post a rating or a comment — it must +# never be able to publish, and the publish path must be able to refuse it. +# +# Existing rows are publish tokens, which is what the default encodes. +class AddKindToApiTokens < ActiveRecord::Migration[8.1] + def change + add_column :api_tokens, :kind, :integer, default: 0, null: false + add_column :device_authorizations, :token_kind, :integer, default: 0, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index c2567d2..7108088 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_08_25_120003) do +ActiveRecord::Schema[8.1].define(version: 2026_08_30_090001) do create_table "active_storage_attachments", force: :cascade do |t| t.bigint "blob_id", null: false t.datetime "created_at", null: false @@ -42,6 +42,7 @@ create_table "api_tokens", force: :cascade do |t| t.datetime "created_at", null: false t.datetime "expires_at", null: false + t.integer "kind", default: 0, null: false t.datetime "last_used_at" t.string "plugin_name" t.json "provenance" @@ -101,6 +102,7 @@ t.string "requested_publisher_name" t.integer "status", default: 0, null: false t.string "token_ciphertext" + t.integer "token_kind", default: 0, null: false t.datetime "updated_at", null: false t.string "user_code", null: false t.integer "user_id" diff --git a/docs/client-api.md b/docs/client-api.md new file mode 100644 index 0000000..b260ff9 --- /dev/null +++ b/docs/client-api.md @@ -0,0 +1,186 @@ +# Client API + +What a signed-in app writes through. Reading a plugin is the anonymous, +cacheable [Browse API](browse-api.md); this is everything that depends on who +is asking — who you are, what you rated, and posting a rating or a comment. + +The desktop [Omarchy plugin browser](https://github.com/jankeesvw/omarchy-plugin-browser) +is the client this exists for. + +## Two kinds of token + +A token that browses is not a token that publishes, and the distinction is +checked at the endpoint rather than inferred from scope. + +| | `publish` | `client` | +|---|---|---| +| Minted by | `omarchy plugin publish` | a signed-in app | +| Can | publish new versions | read as you, rate, comment | +| Cannot | anything else | **publish**, yank, change owners, touch settings | +| Lifetime | 7 days | 30 days | +| Needs a recent second factor | yes | no | +| Budget per account | 25 | 10 | + +A publish token posted to a client endpoint is a `403`, and so is the reverse. +Neither can be mistaken for the other by forgetting to look. + +**Why a client token is not held to the second-factor bar.** Approving one +happens in a browser session that can already rate and comment without ever +proving a second factor. Demanding a passkey before you may leave a comment +would be theatre, and it would lock every account without MFA out of the app +entirely. Publishing is different: it ships code to other people's machines, +so it keeps the bar. The sensitive-change cooldown applies to both — it gates +credential-shaped actions, and minting either of these is one. + +## Signing in + +The device flow ([RFC 8628](https://datatracker.ietf.org/doc/html/rfc8628) +shape), with `scope=client`. + +```sh +curl -X POST https://plugins.omarchy.org/api/v1/device/code -d scope=client +``` + +```json +{ + "device_code": "omd_…", + "user_code": "WDJB-MJHT", + "verification_uri": "https://plugins.omarchy.org/device", + "verification_uri_complete": "https://plugins.omarchy.org/device?code=WDJB-MJHT", + "scope": "client", + "expires_in": 900, + "interval": 5 +} +``` + +Open `verification_uri_complete` in the user's browser — it carries the code, +so an app the user is looking at does not make them retype something it +already knows. Show `user_code` next to it anyway: it is what they check the +page against, and it is the only defence against being walked into approving +somebody else's sign-in. Then poll: + +```sh +curl -X POST https://plugins.omarchy.org/api/v1/device/token -d device_code=omd_… +``` + +| Status | Body | Meaning | +|---|---|---| +| `202` | `{"error": "authorization_pending"}` | Keep polling, no faster than `interval`. | +| `429` | `{"error": "slow_down"}` | You are polling too fast. | +| `403` | `{"error": "access_denied"}` | They pressed Deny. Stop. | +| `400` | `{"error": "expired_token"}` | Expired, or already claimed. Start over. | +| `200` | the token | Store it. | + +```json +{ "token": "omp_…", "token_type": "bearer", "kind": "client", + "scope": "browse, rate and comment as you", + "expires_at": "2026-09-29T09:00:00Z" } +``` + +The token is single-claim: the first poll to succeed gets it and every later +one gets `expired_token`. Store it somewhere only the user can read, and send +it as `Authorization: Bearer omp_…`. + +An unrecognised `scope` asks for `publish`. A caller who asks for something the +registry does not understand gets the flow that is gated hardest, not the one +that is gated least. + +## Responses + +Every response here is `Cache-Control: no-store` — it depends on who is asking +and must never land in a shared cache. Errors are `{"error": "…"}`. + +| Status | When | +|---|---| +| `401` | No token, or it is expired, revoked, or nonsense. | +| `403` | Wrong kind of token, or the account is suspended. | +| `404` | Unknown plugin — and a plugin that has never been public, which is indistinguishable by design. | +| `422` | The rating is out of range, or the comment did not validate. | +| `429` | Rate limited. | + +## `GET /api/v1/me` + +Who the app is talking as. Call it after sign-in and on every launch: it is +how a client finds out its stored token is still good without guessing from a +failed write. + +```json +{ + "user": { "name": "Kim Rivera", "email": "kim@example.com", "admin": false }, + "publishers": [ + { "name": "acme", "kind": "org", "personal": false, "verified": false } + ], + "token": { "hint": "omp_abcd…wxyz", "expires_at": "2026-09-29T09:00:00Z", + "scope": "browse, rate and comment as you" } +} +``` + +`publishers` are the namespaces this account publishes under, accepted +memberships only. The personal one is the account's own handle — a better +answer than anything a client can infer from what is installed locally. + +## `DELETE /api/v1/session` + +Signing out. Revokes **this** token and answers `204`; other devices keep +theirs. Revoking rather than only forgetting matters: a token the client has +thrown away but the registry still honours is exactly the credential nobody +notices leaking. + +## The social payload + +Rating, commenting and deleting all answer with the same shape — the whole +social state of one plugin, after the write. A client applies one response and +never has to stitch two together or refetch to find out what happened. + +```json +{ + "plugin": "acme.weather", + "rating": { "average": 4.5, "count": 10, "mine": 5 }, + "comments": [ + { "id": 12, "body": "Runs well on two monitors.", + "created_at": "2026-08-30T09:00:00Z", "mine": true, + "author": { "name": "Kim Rivera", "publisher_member": true } } + ] +} +``` + +- `rating.mine` is `null` when this account has not rated the plugin. +- `mine` on a comment is whether this account can delete it. Hiding someone + else's is moderation and lives in the admin surfaces, where it leaves an + audit trail. +- `publisher_member` is the badge the web page shows for a comment from the + plugin's own publisher. +- Hidden comments are not in the list. Newest first, fifty at most. + +## `GET /api/v1/plugins//` + +The social payload, unchanged. This is the read; everything below writes. + +## `PUT /api/v1/plugins///rating` + +`value` is 1–5. Idempotent: rating something you have already rated moves your +rating rather than failing on the one-per-user constraint, which is what a +star control does when you click a different star. + +```sh +curl -X PUT -H "Authorization: Bearer omp_…" \ + https://plugins.omarchy.org/api/v1/plugins/acme/weather/rating -d value=5 +``` + +## `DELETE /api/v1/plugins///rating` + +Clears your rating. It has its own verb because "I take it back" is not the +same claim as one star. + +## `POST /api/v1/plugins///comments` + +`body` is 3–2,000 characters. Answers `201` with the social payload. + +Five an hour per token — the same budget the web form gets. A second door onto +the same table must not be the cheap way around the first one's limit. + +## `DELETE /api/v1/comments/` + +Deletes your own comment and answers with the thread it left. Someone else's +is a `404`: the lookup is scoped to your comments, so moderation cannot be +reached from here even by accident. diff --git a/test/integration/client_api_test.rb b/test/integration/client_api_test.rb new file mode 100644 index 0000000..70ebb7b --- /dev/null +++ b/test/integration/client_api_test.rb @@ -0,0 +1,269 @@ +require "test_helper" + +# The API a signed-in desktop plugin browser talks to: sign in through the +# device flow, find out who you are, and write the two things a client can +# write — a rating and a comment. +# +# The line these tests exist to hold is that a client token is not a publish +# token and never becomes one. +class ClientApiTest < ActionDispatch::IntegrationTest + setup do + @user = User.create!(email_address: "kim@example.com", name: "Kim Rivera") + @acme = Publisher.create!(name: "acme", kind: :org) + Membership.create!(publisher: @acme, user: @user, role: :owner, founding: true) + @weather = Plugin.create!(publisher: @acme, name: "weather", summary: "Forecast in the bar", + latest_version: "1.0.0", kinds: [ "bar-widget" ]) + @weather.versions.create!(version: "1.0.0", manifest: {}, sha256: "0" * 64, + size_bytes: 1, state: :published, published_at: 1.day.ago) + end + + def body = response.parsed_body + + def auth(token) = { "Authorization" => "Bearer #{token}" } + + # The whole flow, as the app runs it: ask for a code, open the browser at + # the URL the response hands back, approve, and poll until a token arrives. + def sign_in_client(user: @user) + post "/api/v1/device/code", params: { scope: "client" } + assert_response :created + device_code, user_code = body["device_code"], body["user_code"] + + sign_in_as user, second_factor_verified: false + post approve_device_path, params: { code: user_code } + assert_redirected_to dashboard_path + + post "/api/v1/device/token", params: { device_code: device_code } + assert_response :success + body["token"] + end + + # --- signing in ---------------------------------------------------------- + + test "a client signs in through the device flow and gets a client token" do + post "/api/v1/device/code", params: { scope: "client" } + assert_response :created + assert_equal "client", body["scope"] + # A desktop app can open a page the user only has to approve, rather than + # making them retype a code it already knows. + assert_includes body["verification_uri_complete"], body["user_code"] + + device_code, user_code = body["device_code"], body["user_code"] + + sign_in_as @user, second_factor_verified: false + get device_path(code: user_code) + assert_response :success + assert_match(/plugin browser/i, response.body) + assert_match(/cannot publish/i, response.body) + + post approve_device_path, params: { code: user_code } + assert_redirected_to dashboard_path + + post "/api/v1/device/token", params: { device_code: device_code } + assert_response :success + assert_equal "client", body["kind"] + assert_equal ApiToken::CLIENT_TTL.from_now.to_date.to_s, Date.parse(body["expires_at"]).to_s + end + + # A publish token can ship code, so it needs a freshly proved second factor. + # A client token can do nothing this browser session cannot already do + # without one, and gating it would lock everyone without MFA out of the app. + test "signing a client in does not demand a second factor" do + refute @user.second_factor? + assert sign_in_client.start_with?("omp_") + end + + test "a publish token still demands a second factor" do + post "/api/v1/device/code" + user_code = body["user_code"] + + sign_in_as @user, second_factor_verified: false + post approve_device_path, params: { code: user_code } + assert_redirected_to settings_two_factor_path + end + + # The scope is decided when the row is created, so nothing the approving + # browser sends can turn a client request into a publish token. + test "an unknown scope asks for the flow that is gated hardest" do + post "/api/v1/device/code", params: { scope: "everything" } + assert_equal "publish", body["scope"] + end + + test "me reports the account and the namespaces it publishes under" do + get "/api/v1/me", headers: auth(sign_in_client) + assert_response :success + + assert_equal "Kim Rivera", body["user"]["name"] + refute body["user"]["admin"] + assert_equal [ "acme" ], body["publishers"].map { |p| p["name"] } + assert_equal "browse, rate and comment as you", body["token"]["scope"] + assert_equal "no-store", response.headers["Cache-Control"] + end + + test "signing out revokes the token rather than only forgetting it" do + token = sign_in_client + + delete "/api/v1/session", headers: auth(token) + assert_response :no_content + + get "/api/v1/me", headers: auth(token) + assert_response :unauthorized + end + + # --- the boundary between the two kinds ----------------------------------- + + test "a client token cannot publish" do + post "/api/v1/plugins/acme/weather/versions", params: TarballBuilder.build, + headers: auth(sign_in_client).merge("Content-Type" => "application/gzip") + assert_response :forbidden + assert_match(/not a publish token/, body["error"]) + end + + test "a publish token cannot comment" do + publish_token = ApiToken.mint!(user: @user).plaintext_token + + post "/api/v1/plugins/acme/weather/comments", params: { body: "Nice one" }, + headers: auth(publish_token) + assert_response :forbidden + assert_match(/not a client token/, body["error"]) + assert_equal 0, @weather.comments.count + end + + test "a suspended account loses a live client token" do + token = sign_in_client + @user.update!(suspended_at: Time.current) + + get "/api/v1/me", headers: auth(token) + assert_response :forbidden + assert_match(/suspended/, body["error"]) + end + + test "no token at all is unauthorized" do + get "/api/v1/me" + assert_response :unauthorized + end + + # --- rating --------------------------------------------------------------- + + test "rating a plugin, moving the rating, and clearing it" do + token = sign_in_client + + put "/api/v1/plugins/acme/weather/rating", params: { value: 5 }, headers: auth(token) + assert_response :success + assert_equal 5, body["rating"]["mine"] + assert_equal 5.0, body["rating"]["average"] + assert_equal 1, body["rating"]["count"] + + # Clicking a different star moves the rating rather than failing on the + # one-per-user constraint. + put "/api/v1/plugins/acme/weather/rating", params: { value: 3 }, headers: auth(token) + assert_response :success + assert_equal 3, body["rating"]["mine"] + assert_equal 1, body["rating"]["count"] + + delete "/api/v1/plugins/acme/weather/rating", headers: auth(token) + assert_response :success + assert_nil body["rating"]["mine"] + assert_equal 0, body["rating"]["count"] + end + + test "a rating outside one to five is refused" do + token = sign_in_client + [ 0, 6, -1 ].each do |value| + put "/api/v1/plugins/acme/weather/rating", params: { value: value }, headers: auth(token) + assert_response :unprocessable_entity + end + assert_equal 0, @weather.ratings.count + end + + # --- comments ------------------------------------------------------------- + + test "posting a comment answers with the thread it landed in" do + token = sign_in_client + + post "/api/v1/plugins/acme/weather/comments", params: { body: "Runs well on two monitors." }, + headers: auth(token) + assert_response :created + + comment = body["comments"].sole + assert_equal "Runs well on two monitors.", comment["body"] + assert_equal "Kim Rivera", comment["author"]["name"] + # Kim owns acme, so the badge the web page shows is on the API too. + assert comment["author"]["publisher_member"] + assert comment["mine"], "the author must be able to see it is theirs" + end + + test "an empty comment is refused with the reason" do + post "/api/v1/plugins/acme/weather/comments", params: { body: " " }, + headers: auth(sign_in_client) + assert_response :unprocessable_entity + assert_match(/body/i, body["error"]) + end + + test "authors delete their own comments and nobody else's" do + mine = sign_in_client + comment = @weather.comments.create!(user: @user, body: "My own comment") + + stranger = User.create!(email_address: "someone@example.com", name: "Someone") + theirs = @weather.comments.create!(user: stranger, body: "Someone else's comment") + + delete "/api/v1/comments/#{theirs.id}", headers: auth(mine) + assert_response :not_found + assert Comment.exists?(theirs.id), "deleting someone else's comment must not work" + + delete "/api/v1/comments/#{comment.id}", headers: auth(mine) + assert_response :success + refute Comment.exists?(comment.id) + assert_equal [ theirs.id ], body["comments"].map { |c| c["id"] } + end + + test "a hidden comment is not in the thread" do + token = sign_in_client + @weather.comments.create!(user: @user, body: "Visible comment") + @weather.comments.create!(user: @user, body: "Hidden comment", hidden_at: Time.current) + + get "/api/v1/plugins/acme/weather", headers: auth(token) + assert_response :success + assert_equal [ "Visible comment" ], body["comments"].map { |c| c["body"] } + end + + # The web form allows five comments an hour. A second door onto the same + # table must not be the cheap way around the first one's limit. + test "commenting is rate limited" do + token = sign_in_client + + # Rate limiting counts in Rails.cache, which is a null store in test. + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + begin + 6.times do |i| + post "/api/v1/plugins/acme/weather/comments", params: { body: "Comment number #{i}" }, + headers: auth(token) + end + assert_response :too_many_requests + assert_equal 5, @weather.comments.count + ensure + Rails.cache = original_cache + end + end + + # --- what a client may see ------------------------------------------------ + + test "a plugin that has never been public is not there to read or write" do + unreleased = Plugin.create!(publisher: Publisher.create!(name: "quiet", kind: :org), + name: "secret", summary: "Not out yet", kinds: [ "bar-widget" ]) + assert_not unreleased.ever_public? + + token = sign_in_client + get "/api/v1/plugins/quiet/secret", headers: auth(token) + assert_response :not_found + + post "/api/v1/plugins/quiet/secret/comments", params: { body: "Hello there" }, headers: auth(token) + assert_response :not_found + assert_equal 0, unreleased.comments.count + end + + test "an unknown plugin is a plain not found" do + get "/api/v1/plugins/acme/nothing", headers: auth(sign_in_client) + assert_response :not_found + end +end From 4f29d0d42c46cb15536a084a8578376983fa6b43 Mon Sep 17 00:00:00 2001 From: Jankees van Woezik Date: Sun, 30 Aug 2026 09:21:01 +0200 Subject: [PATCH 03/10] Count a plugin's comments on the listing entry A card shows the comment count next to the rating, and a grid cannot afford to fetch a thread per card to work it out. It is the counter cache, so it costs nothing; the thread itself stays on the plugin response. --- app/views/plugins/_plugin.json.jbuilder | 4 ++++ docs/browse-api.md | 5 +++++ test/integration/browse_api_test.rb | 11 +++++++++++ 3 files changed, 20 insertions(+) diff --git a/app/views/plugins/_plugin.json.jbuilder b/app/views/plugins/_plugin.json.jbuilder index 13959b7..5a87e43 100644 --- a/app/views/plugins/_plugin.json.jbuilder +++ b/app/views/plugins/_plugin.json.jbuilder @@ -30,6 +30,10 @@ json.rating do json.average plugin.average_rating json.count plugin.ratings_count end +# The counter cache, not a query — a card shows "12 comments" next to the +# rating, and fetching a thread per card to count it is not a thing a grid can +# afford. The thread itself is on the detail response. +json.comments plugin.comments_count # Selected only by the directory queries — absent elsewhere rather than faked. json.first_published_at plugin.try(:first_published_at) diff --git a/docs/browse-api.md b/docs/browse-api.md index fdd283f..c92adbb 100644 --- a/docs/browse-api.md +++ b/docs/browse-api.md @@ -78,6 +78,10 @@ category:system curated category } ``` +`comments` is how many visible comments a plugin has, so a grid can show the +count without fetching a thread per card. The thread itself is on the plugin +response. + `taxonomy` is the curated browse vocabulary with live counts. Render facets from it rather than hardcoding a copy — categories and tags are a governance decision and the list changes without warning. @@ -143,6 +147,7 @@ One plugin, with everything the web page renders. "downloads": 500, "views": 12, "rating": { "average": 4.5, "count": 10 }, + "comments": 3, "repository": { "url": "https://github.com/acme/weather", "label": "GitHub", "stars": 42, diff --git a/test/integration/browse_api_test.rb b/test/integration/browse_api_test.rb index cfdb665..3172932 100644 --- a/test/integration/browse_api_test.rb +++ b/test/integration/browse_api_test.rb @@ -32,6 +32,15 @@ def seed_filler(count) # --- directory ----------------------------------------------------------- + test "the comment count on a listing entry follows the thread" do + @weather.comments.create!(user: User.create!(email_address: "kim@example.com", name: "Kim"), + body: "Runs well on two monitors.") + + get directory_json_path + assert_response :success + assert_equal 1, body["plugins"].sole["comments"] + end + # A namespace seeded from the legacy marketplace has nobody behind it yet, # and a client must be able to say so instead of implying endorsement. test "an unclaimed publisher is said so on the listing entry" do @@ -63,6 +72,8 @@ def seed_filler(count) assert_equal "widgets", entry["category"] assert_equal "Widgets", entry["category_label"] assert_equal "omarchy plugin add acme/weather", entry["install_command"] + # Counted, not fetched — a grid cannot afford a thread per card. + assert_equal 0, entry["comments"] assert_equal "http://registry.test/plugins/acme/weather", entry["url"] # The namespace's standing travels with every entry, so a grid can render From 4a7745a3b33f740c6ffd1332c9e60917a32652aa Mon Sep 17 00:00:00 2001 From: Jankees van Woezik Date: Sun, 30 Aug 2026 09:41:45 +0200 Subject: [PATCH 04/10] Key the comment budget on the account, not the token Signing in again would otherwise reset it, which makes the limit a formality. A bearer token also has no business being part of a cache key, which is what it was. --- app/controllers/api/v1/comments_controller.rb | 7 ++++- test/integration/client_api_test.rb | 26 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/v1/comments_controller.rb b/app/controllers/api/v1/comments_controller.rb index 1b7ed08..83ca934 100644 --- a/app/controllers/api/v1/comments_controller.rb +++ b/app/controllers/api/v1/comments_controller.rb @@ -9,8 +9,13 @@ class CommentsController < BaseController # The same budget the web form gets. A second door onto the same table # must not be the cheap way around the first one's limit. + # + # Keyed on the account, not the token: signing in again would otherwise + # reset the budget, and a bearer token has no business being part of a + # cache key. Declared after the authentication filter so current_user is + # there to read. rate_limit to: 5, within: 1.hour, only: :create, store: RATE_LIMIT_STORE, - by: -> { request.authorization.to_s }, + by: -> { current_user&.id }, with: -> { render json: { error: "slow down — try again in a bit" }, status: :too_many_requests } def create diff --git a/test/integration/client_api_test.rb b/test/integration/client_api_test.rb index 70ebb7b..50d11ce 100644 --- a/test/integration/client_api_test.rb +++ b/test/integration/client_api_test.rb @@ -246,6 +246,32 @@ def sign_in_client(user: @user) end end + # The budget belongs to the account, not to the credential — otherwise + # signing in again is the cheap way to reset it. + test "signing in again does not reset the comment budget" do + first = sign_in_client + second = sign_in_client + refute_equal first, second, "the two sign-ins should be different tokens" + + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + begin + 3.times do |i| + post "/api/v1/plugins/acme/weather/comments", params: { body: "First token #{i}" }, + headers: auth(first) + assert_response :created + end + 3.times do |i| + post "/api/v1/plugins/acme/weather/comments", params: { body: "Second token #{i}" }, + headers: auth(second) + end + assert_response :too_many_requests + assert_equal 5, @weather.comments.count + ensure + Rails.cache = original_cache + end + end + # --- what a client may see ------------------------------------------------ test "a plugin that has never been public is not there to read or write" do From 706bbafadc31359588b60ca22a01e6b65ee0f2fb Mon Sep 17 00:00:00 2001 From: Jankees van Woezik Date: Sun, 30 Aug 2026 09:46:47 +0200 Subject: [PATCH 05/10] Ask the second-factor gate exactly the question it is for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It guards minting something that can ship code. It was standing in front of the whole approve action, so three things that mint nothing were being asked for a passkey they had no business needing. Denying, first: someone who sees a code they did not ask for must be able to say no immediately, and gating the safe direction is backwards. A client sign-in, which can only do what the approving browser session can already do without a second factor. And a code that expired or was already answered, where the action reports that itself — telling someone to go and set up MFA when what happened is that their code ran out answers the wrong question. --- app/controllers/device_controller.rb | 28 ++++++++++++++++++++++------ test/integration/client_api_test.rb | 25 +++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/app/controllers/device_controller.rb b/app/controllers/device_controller.rb index 76dd593..25dd68c 100644 --- a/app/controllers/device_controller.rb +++ b/app/controllers/device_controller.rb @@ -9,7 +9,7 @@ # the app entirely. The sensitive-change cooldown still applies to both: it # gates credential-shaped actions, and this is one. class DeviceController < ApplicationController - before_action :require_recent_second_factor, only: :approve, unless: :approving_client? + before_action :require_recent_second_factor, only: :approve, if: :minting_publish_token? before_action :require_no_sensitive_cooldown, only: :approve def show @@ -50,10 +50,26 @@ def approve private - # Reads the pending authorization rather than anything the request says, so - # the weaker gate can only ever be reached by a row that was created asking - # for a client token. - def approving_client? - DeviceAuthorization.find_by_user_code(params[:code])&.for_client? || false + # The second-factor gate exists to protect minting something that can ship + # code, so it asks exactly that question and stays out of the way otherwise. + # + # Three things are not that, and each was being asked for a passkey it had no + # business needing: + # + # - Denying. Someone who sees a code they did not ask for must be able to say + # no immediately; gating the safe direction is backwards. + # - A client sign-in, which can only do what this browser session can already + # do without a second factor. + # - A code that has expired or was already answered, where nothing will be + # minted at all and the action says so itself. Demanding a factor first + # answers the wrong question, and tells someone to go set up MFA when what + # actually happened is that their code ran out. + # + # It reads the pending authorization rather than anything the request says + # about scope, so the weaker path can only ever be reached by a row that was + # created asking for a client token. + def minting_publish_token? + return false if params[:decision] == "deny" + DeviceAuthorization.find_by_user_code(params[:code])&.for_publish? || false end end diff --git a/test/integration/client_api_test.rb b/test/integration/client_api_test.rb index 50d11ce..6ada6fe 100644 --- a/test/integration/client_api_test.rb +++ b/test/integration/client_api_test.rb @@ -81,6 +81,31 @@ def sign_in_client(user: @user) assert_redirected_to settings_two_factor_path end + # Someone who sees a code they did not ask for must be able to say no + # immediately. Gating the safe direction behind a passkey is backwards. + test "denying never asks for a second factor" do + post "/api/v1/device/code" + device_code, user_code = body["device_code"], body["user_code"] + + sign_in_as @user, second_factor_verified: false + post approve_device_path, params: { code: user_code, decision: "deny" } + assert_redirected_to dashboard_path + + post "/api/v1/device/token", params: { device_code: device_code } + assert_response :forbidden + assert_equal "access_denied", body["error"] + end + + # Nothing is minted for a code that has run out, so demanding a factor first + # answers the wrong question — and tells someone to go and set up MFA when + # what actually happened is that their code expired. + test "an expired code says so rather than demanding a second factor" do + sign_in_as @user, second_factor_verified: false + post approve_device_path, params: { code: "ZZZZ-ZZZZ" } + assert_redirected_to device_path + assert_match(/expired/i, flash[:alert]) + end + # The scope is decided when the row is created, so nothing the approving # browser sends can turn a client request into a publish token. test "an unknown scope asks for the flow that is gated hardest" do From 93feaf643fa952d0b8e3e6456caaaa086c1ff68e Mon Sep 17 00:00:00 2001 From: Jankees van Woezik Date: Sun, 30 Aug 2026 10:08:08 +0200 Subject: [PATCH 06/10] Fix three things a review found in the client API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Denying was blocked by the sensitive-change cooldown.** The second-factor gate already skipped it — saying no mints nothing, and gating the safe direction is backwards — but the cooldown did not. An account in the cooldown is exactly the one most likely to be looking at a code it did not ask for, and it could not refuse until the code expired on its own. **comments_count counted comments nobody can read.** It was a counter cache, and hiding a comment is a soft delete, so a moderated thread left a card claiming more than it would show. Comment maintains it under the plugin lock now, the way Rating already keeps its totals honest, and a migration brings the existing rows in line with what the column means. **One key meant two things.** The listing partial published the count as `comments`, and the detail response layers a `comments` ARRAY over that same partial — so a plugin parsed as a number in one response and a list in the other, depending on where a client found it. The count is `comments_count` now, in both the browse and the client API, and the social payload carries it so a client never has to count a list that was truncated at fifty. --- .../concerns/api/v1/plugin_scoped.rb | 5 +++ app/controllers/device_controller.rb | 13 ++++-- app/models/comment.rb | 15 ++++++- app/views/plugins/_plugin.json.jbuilder | 12 +++-- ...60830100001_count_only_visible_comments.rb | 26 +++++++++++ db/schema.rb | 2 +- docs/browse-api.md | 11 +++-- docs/client-api.md | 15 +++++-- test/integration/browse_api_test.rb | 18 +++++++- test/integration/client_api_test.rb | 44 ++++++++++++++++++- 10 files changed, 141 insertions(+), 20 deletions(-) create mode 100644 db/migrate/20260830100001_count_only_visible_comments.rb diff --git a/app/controllers/concerns/api/v1/plugin_scoped.rb b/app/controllers/concerns/api/v1/plugin_scoped.rb index 4bc53fa..5726b5e 100644 --- a/app/controllers/concerns/api/v1/plugin_scoped.rb +++ b/app/controllers/concerns/api/v1/plugin_scoped.rb @@ -36,6 +36,11 @@ def social_payload(plugin = @plugin) { plugin: plugin.manifest_id, + # How long the thread actually is, which is not `comments.length` + # when it has been truncated. A client showing a count next to a + # card needs the real number, or a plugin with eighty comments + # starts claiming fifty the moment someone opens it. + comments_count: plugin.comments_count, rating: { average: plugin.average_rating, count: plugin.ratings_count, diff --git a/app/controllers/device_controller.rb b/app/controllers/device_controller.rb index 25dd68c..9da74b7 100644 --- a/app/controllers/device_controller.rb +++ b/app/controllers/device_controller.rb @@ -10,7 +10,7 @@ # gates credential-shaped actions, and this is one. class DeviceController < ApplicationController before_action :require_recent_second_factor, only: :approve, if: :minting_publish_token? - before_action :require_no_sensitive_cooldown, only: :approve + before_action :require_no_sensitive_cooldown, only: :approve, unless: :denying? def show @user_code = params[:code] @@ -57,7 +57,8 @@ def approve # business needing: # # - Denying. Someone who sees a code they did not ask for must be able to say - # no immediately; gating the safe direction is backwards. + # no immediately; gating the safe direction is backwards. The cooldown + # skips it for the same reason. # - A client sign-in, which can only do what this browser session can already # do without a second factor. # - A code that has expired or was already answered, where nothing will be @@ -69,7 +70,13 @@ def approve # about scope, so the weaker path can only ever be reached by a row that was # created asking for a client token. def minting_publish_token? - return false if params[:decision] == "deny" + return false if denying? DeviceAuthorization.find_by_user_code(params[:code])&.for_publish? || false end + + # Saying no mints nothing, so neither gate applies. An account in the + # sensitive-change cooldown is exactly the one most likely to be looking at + # a code it did not ask for, and leaving it unable to refuse until the code + # expires on its own is the wrong way round. + def denying? = params[:decision] == "deny" end diff --git a/app/models/comment.rb b/app/models/comment.rb index b8459f7..f46af08 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -1,7 +1,7 @@ # No anonymous comments — registry accounts only, which alone kills most of # the moderation tarpit. Reports land in the shared admin queue. class Comment < ApplicationRecord - belongs_to :plugin, counter_cache: true + belongs_to :plugin belongs_to :user has_many :reports, as: :reportable, dependent: :destroy @@ -9,11 +9,24 @@ class Comment < ApplicationRecord scope :visible, -> { where(hidden_at: nil) } + # plugins.comments_count is the count of VISIBLE comments, which a plain + # counter cache cannot be: hiding is a soft delete, so the cache would keep + # counting a comment nobody can read and a card would claim a thread longer + # than the one it opens. Recomputed under the plugin lock instead, the same + # way Rating keeps its totals honest. + after_commit :refresh_plugin_comment_count + def hidden? = hidden_at.present? # Comments from the plugin's own publisher get a badge def from_publisher? = user.member_of?(plugin.publisher) + def refresh_plugin_comment_count + plugin.with_lock do + plugin.update_columns(comments_count: plugin.comments.visible.count) + end + end + def hide!(actor:) update!(hidden_at: Time.current) AuditEvent.record!(actor:, action: "comment.hide", subject: self, diff --git a/app/views/plugins/_plugin.json.jbuilder b/app/views/plugins/_plugin.json.jbuilder index 5a87e43..6df2200 100644 --- a/app/views/plugins/_plugin.json.jbuilder +++ b/app/views/plugins/_plugin.json.jbuilder @@ -30,10 +30,14 @@ json.rating do json.average plugin.average_rating json.count plugin.ratings_count end -# The counter cache, not a query — a card shows "12 comments" next to the -# rating, and fetching a thread per card to count it is not a thing a grid can -# afford. The thread itself is on the detail response. -json.comments plugin.comments_count +# The counter, not a query — a card shows "12 comments" next to the rating, and +# fetching a thread per card to count it is not a thing a grid can afford. +# +# `comments_count` rather than `comments` because the detail response layers a +# `comments` ARRAY on top of this same partial. One key meaning a number in one +# response and a list in another is how a client ends up parsing a plugin two +# different ways depending on where it found it. +json.comments_count plugin.comments_count # Selected only by the directory queries — absent elsewhere rather than faked. json.first_published_at plugin.try(:first_published_at) diff --git a/db/migrate/20260830100001_count_only_visible_comments.rb b/db/migrate/20260830100001_count_only_visible_comments.rb new file mode 100644 index 0000000..e492e8f --- /dev/null +++ b/db/migrate/20260830100001_count_only_visible_comments.rb @@ -0,0 +1,26 @@ +# plugins.comments_count now counts VISIBLE comments. +# +# It was a counter cache, and hiding a comment is a soft delete, so a hidden +# one went on being counted: a card claimed a thread longer than the one it +# opened. Comment maintains it under the plugin lock now, the way Rating keeps +# its totals honest — this brings the existing rows in line with what the +# column means from here. +class CountOnlyVisibleComments < ActiveRecord::Migration[8.1] + def up + execute <<~SQL.squish + UPDATE plugins SET comments_count = ( + SELECT COUNT(*) FROM comments + WHERE comments.plugin_id = plugins.id AND comments.hidden_at IS NULL + ) + SQL + end + + # The old meaning was every comment, hidden or not. + def down + execute <<~SQL.squish + UPDATE plugins SET comments_count = ( + SELECT COUNT(*) FROM comments WHERE comments.plugin_id = plugins.id + ) + SQL + end +end diff --git a/db/schema.rb b/db/schema.rb index 7108088..8660bb0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_08_30_090001) do +ActiveRecord::Schema[8.1].define(version: 2026_08_30_100001) do create_table "active_storage_attachments", force: :cascade do |t| t.bigint "blob_id", null: false t.datetime "created_at", null: false diff --git a/docs/browse-api.md b/docs/browse-api.md index c92adbb..fd261bc 100644 --- a/docs/browse-api.md +++ b/docs/browse-api.md @@ -78,9 +78,12 @@ category:system curated category } ``` -`comments` is how many visible comments a plugin has, so a grid can show the -count without fetching a thread per card. The thread itself is on the plugin -response. +`comments_count` is how many **visible** comments a plugin has — hidden ones +are not counted, so it agrees with the thread rather than with what moderation +has taken down. It is there so a grid can show the count without fetching a +thread per card. The thread itself is `comments` on the plugin response, which +is an array; the count keeps its own name so one key never means a number in +one response and a list in another. `taxonomy` is the curated browse vocabulary with live counts. Render facets from it rather than hardcoding a copy — categories and tags are a governance @@ -147,7 +150,7 @@ One plugin, with everything the web page renders. "downloads": 500, "views": 12, "rating": { "average": 4.5, "count": 10 }, - "comments": 3, + "comments_count": 3, "repository": { "url": "https://github.com/acme/weather", "label": "GitHub", "stars": 42, diff --git a/docs/client-api.md b/docs/client-api.md index b260ff9..6e8a5e1 100644 --- a/docs/client-api.md +++ b/docs/client-api.md @@ -30,7 +30,9 @@ proving a second factor. Demanding a passkey before you may leave a comment would be theatre, and it would lock every account without MFA out of the app entirely. Publishing is different: it ships code to other people's machines, so it keeps the bar. The sensitive-change cooldown applies to both — it gates -credential-shaped actions, and minting either of these is one. +credential-shaped actions, and minting either of these is one. Neither applies +to pressing **Deny**: that mints nothing, and an account in the cooldown is +exactly the one most likely to be looking at a code it did not ask for. ## Signing in @@ -135,6 +137,7 @@ never has to stitch two together or refetch to find out what happened. ```json { "plugin": "acme.weather", + "comments_count": 3, "rating": { "average": 4.5, "count": 10, "mine": 5 }, "comments": [ { "id": 12, "body": "Runs well on two monitors.", @@ -151,6 +154,11 @@ never has to stitch two together or refetch to find out what happened. - `publisher_member` is the badge the web page shows for a comment from the plugin's own publisher. - Hidden comments are not in the list. Newest first, fifty at most. +- `comments_count` is how long the thread actually is, which is not the length + of `comments` once it has been truncated. Show that number, not the array's + length, or a plugin with eighty comments starts claiming fifty the moment + somebody opens it. It counts visible comments only, so it agrees with the + list rather than with what moderation has hidden. ## `GET /api/v1/plugins//` @@ -176,8 +184,9 @@ same claim as one star. `body` is 3–2,000 characters. Answers `201` with the social payload. -Five an hour per token — the same budget the web form gets. A second door onto -the same table must not be the cheap way around the first one's limit. +Five an hour per **account** — the same budget the web form gets. A second door +onto the same table must not be the cheap way around the first one's limit, and +counting per token would make signing in again the cheap way around this one. ## `DELETE /api/v1/comments/` diff --git a/test/integration/browse_api_test.rb b/test/integration/browse_api_test.rb index 3172932..db3923e 100644 --- a/test/integration/browse_api_test.rb +++ b/test/integration/browse_api_test.rb @@ -32,13 +32,27 @@ def seed_filler(count) # --- directory ----------------------------------------------------------- + # The detail response layers a comments ARRAY over the same partial, so the + # count needs its own name or a client parses a plugin two different ways + # depending on where it found it. + test "the count and the thread do not share a key" do + kim = User.create!(email_address: "kim@example.com", name: "Kim") + @weather.comments.create!(user: kim, body: "Runs well on two monitors.") + + get plugin_path("acme", "weather", format: :json) + assert_response :success + plugin = body["plugin"] + assert_kind_of Array, plugin["comments"] + assert_equal 1, plugin["comments_count"] + end + test "the comment count on a listing entry follows the thread" do @weather.comments.create!(user: User.create!(email_address: "kim@example.com", name: "Kim"), body: "Runs well on two monitors.") get directory_json_path assert_response :success - assert_equal 1, body["plugins"].sole["comments"] + assert_equal 1, body["plugins"].sole["comments_count"] end # A namespace seeded from the legacy marketplace has nobody behind it yet, @@ -73,7 +87,7 @@ def seed_filler(count) assert_equal "Widgets", entry["category_label"] assert_equal "omarchy plugin add acme/weather", entry["install_command"] # Counted, not fetched — a grid cannot afford a thread per card. - assert_equal 0, entry["comments"] + assert_equal 0, entry["comments_count"] assert_equal "http://registry.test/plugins/acme/weather", entry["url"] # The namespace's standing travels with every entry, so a grid can render diff --git a/test/integration/client_api_test.rb b/test/integration/client_api_test.rb index 6ada6fe..1212c5b 100644 --- a/test/integration/client_api_test.rb +++ b/test/integration/client_api_test.rb @@ -106,6 +106,24 @@ def sign_in_client(user: @user) assert_match(/expired/i, flash[:alert]) end + # An account in the sensitive-change cooldown is exactly the one most likely + # to be looking at a code it did not ask for. Leaving it unable to refuse + # until the code expires on its own is the wrong way round. + test "denying works during the sensitive-change cooldown" do + post "/api/v1/device/code" + device_code, user_code = body["device_code"], body["user_code"] + + @user.update!(sensitive_change_at: Time.current) + assert @user.in_publish_cooldown? + + sign_in_as @user, second_factor_verified: false + post approve_device_path, params: { code: user_code, decision: "deny" } + assert_redirected_to dashboard_path + + post "/api/v1/device/token", params: { device_code: device_code } + assert_equal "access_denied", body["error"] + end + # The scope is decided when the row is created, so nothing the approving # browser sends can turn a client request into a publish token. test "an unknown scope asks for the flow that is gated hardest" do @@ -241,14 +259,36 @@ def sign_in_client(user: @user) assert_equal [ theirs.id ], body["comments"].map { |c| c["id"] } end - test "a hidden comment is not in the thread" do + test "a hidden comment is neither in the thread nor in the count" do token = sign_in_client @weather.comments.create!(user: @user, body: "Visible comment") - @weather.comments.create!(user: @user, body: "Hidden comment", hidden_at: Time.current) + hidden = @weather.comments.create!(user: @user, body: "Hidden comment") + + get "/api/v1/plugins/acme/weather", headers: auth(token) + assert_equal 2, body["comments_count"], "both are visible so far" + + hidden.hide!(actor: @user) get "/api/v1/plugins/acme/weather", headers: auth(token) assert_response :success assert_equal [ "Visible comment" ], body["comments"].map { |c| c["body"] } + # The count has to follow the thread. A card claiming two next to a thread + # of one is the same bug read from the other side. + assert_equal 1, body["comments_count"] + assert_equal 1, @weather.reload.comments_count + end + + # The thread is truncated; the count is not. A client showing the array's + # length would make a busy plugin look quieter every time somebody opened it. + test "the count is the whole thread, not the page of it that came back" do + token = sign_in_client + limit = Api::V1::PluginScoped::COMMENT_LIMIT + (limit + 3).times { |i| @weather.comments.create!(user: @user, body: "Comment number #{i}") } + + get "/api/v1/plugins/acme/weather", headers: auth(token) + assert_response :success + assert_equal limit, body["comments"].length + assert_equal limit + 3, body["comments_count"] end # The web form allows five comments an hour. A second door onto the same From b2a33fb25303e437edd1d1d9f59df1e7cfe990db Mon Sep 17 00:00:00 2001 From: Jankees van Woezik Date: Sun, 30 Aug 2026 10:12:13 +0200 Subject: [PATCH 07/10] Make the web form and the app share one comment budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment on the client API said "the same budget the web form gets", and it was not: Rails' rate_limit keys its counter on the controller, so two controllers can never share one however their limits are declared. An account had five an hour on each door and ten across both. Counted by hand against a key that names the account and nothing else, so it is genuinely one budget. By the account rather than the IP, for the reason OnboardingController already gives about its own limit — an office behind one address must not share one — and commenting needs an account either way. Also stops a first rating racing its own unique index. Two clicks landing together both see no row and both insert, and the loser got a 500 for an ordinary request; losing that race means the row exists, so the retry updates it, which is the answer either click deserved. --- app/controllers/api/v1/comments_controller.rb | 23 ++++++------ app/controllers/api/v1/ratings_controller.rb | 22 ++++++++++- app/controllers/comments_controller.rb | 12 +++++- .../concerns/comment_rate_limit.rb | 30 +++++++++++++++ test/integration/client_api_test.rb | 37 +++++++++++++++++++ 5 files changed, 109 insertions(+), 15 deletions(-) create mode 100644 app/controllers/concerns/comment_rate_limit.rb diff --git a/app/controllers/api/v1/comments_controller.rb b/app/controllers/api/v1/comments_controller.rb index 83ca934..d3429d7 100644 --- a/app/controllers/api/v1/comments_controller.rb +++ b/app/controllers/api/v1/comments_controller.rb @@ -2,22 +2,14 @@ module Api module V1 class CommentsController < BaseController include PluginScoped + include CommentRateLimit before_action :authenticate_client_token! + # After authentication, because the budget is keyed on the account. + before_action :enforce_comment_budget, only: :create before_action :load_plugin!, only: :create after_action { response.headers["Cache-Control"] = "no-store" } - # The same budget the web form gets. A second door onto the same table - # must not be the cheap way around the first one's limit. - # - # Keyed on the account, not the token: signing in again would otherwise - # reset the budget, and a bearer token has no business being part of a - # cache key. Declared after the authentication filter so current_user is - # there to read. - rate_limit to: 5, within: 1.hour, only: :create, store: RATE_LIMIT_STORE, - by: -> { current_user&.id }, - with: -> { render json: { error: "slow down — try again in a bit" }, status: :too_many_requests } - def create comment = @plugin.comments.new(user: current_user, body: params[:body]) if comment.save @@ -36,6 +28,15 @@ def destroy comment.destroy! render json: social_payload(plugin) end + + private + + # Literally the same budget as the web form, not a second one that + # happens to be the same size — see CommentRateLimit. + def enforce_comment_budget + return unless comment_budget_exceeded? + render json: { error: "slow down — try again in a bit" }, status: :too_many_requests + end end end end diff --git a/app/controllers/api/v1/ratings_controller.rb b/app/controllers/api/v1/ratings_controller.rb index 8317b11..133f0a1 100644 --- a/app/controllers/api/v1/ratings_controller.rb +++ b/app/controllers/api/v1/ratings_controller.rb @@ -16,8 +16,7 @@ def update return render json: { error: "value must be between 1 and 5" }, status: :unprocessable_entity end - rating = @plugin.ratings.find_or_initialize_by(user: current_user) - rating.update!(value: value) + upsert_rating(value) render json: social_payload end @@ -27,6 +26,25 @@ def destroy @plugin.ratings.find_by(user: current_user)&.destroy! render json: social_payload end + + private + + # find-then-write races the one-rating-per-user index: two clicks landing + # together both see no row and both insert, and the loser gets a 500 for + # what is a perfectly ordinary request. Losing that race means the row + # now exists, so the retry finds it and updates — which is the answer + # either click deserved. + # + # Not covered by a test: reaching it means suspending one request between + # its find and its write, and a test that fakes that convincingly enough + # to be worth reading has not suggested itself. The web form has the same + # shape and the same exposure. + def upsert_rating(value) + rating = @plugin.ratings.find_or_initialize_by(user: current_user) + rating.update!(value: value) + rescue ActiveRecord::RecordNotUnique + @plugin.ratings.find_by!(user: current_user).update!(value: value) + end end end end diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 0de279e..e4369f2 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -1,6 +1,7 @@ class CommentsController < ApplicationController - rate_limit to: 5, within: 1.hour, only: :create, - with: -> { redirect_back fallback_location: root_path, alert: "Slow down — try again in a bit." } + include CommentRateLimit + + before_action :enforce_comment_budget, only: :create def create plugin = find_plugin @@ -22,6 +23,13 @@ def destroy private + # Shared with the client API, so posting through an app is not the cheap way + # around the form's limit. + def enforce_comment_budget + return unless comment_budget_exceeded? + redirect_back fallback_location: root_path, alert: "Slow down — try again in a bit." + end + def find_plugin Publisher.find_by!(name: params[:publisher]).plugins.find_by!(name: params[:name]) end diff --git a/app/controllers/concerns/comment_rate_limit.rb b/app/controllers/concerns/comment_rate_limit.rb new file mode 100644 index 0000000..6d4393e --- /dev/null +++ b/app/controllers/concerns/comment_rate_limit.rb @@ -0,0 +1,30 @@ +# One comment budget per account, however they reach the table. +# +# There are two doors onto comments — the form on the plugin page and the +# client API a signed-in app posts through — and Rails' `rate_limit` builds its +# cache key from the controller, so two controllers can never share one budget +# no matter how their limits are declared. Left as two, the account gets five +# an hour on each and ten in total, which is not what either comment said. +# +# So the counting happens by hand against a key that names the account and +# nothing else. By the account rather than the IP, for the reason +# OnboardingController already gives about its own limit: an office full of +# people behind one address must not share a budget. Commenting requires an +# account either way, so there is always one to key on. +module CommentRateLimit + extend ActiveSupport::Concern + + MAX_PER_WINDOW = 5 + WINDOW = 1.hour + + private + + def comment_budget_exceeded? + return false unless Current.user + key = "comment-budget:#{Current.user.id}" + # A store that counts nothing (the null store in test) reads as under + # budget, which is the same thing Rails' own rate_limit does. + count = Rails.cache.increment(key, 1, expires_in: WINDOW) + count.present? && count > MAX_PER_WINDOW + end +end diff --git a/test/integration/client_api_test.rb b/test/integration/client_api_test.rb index 1212c5b..6173d1f 100644 --- a/test/integration/client_api_test.rb +++ b/test/integration/client_api_test.rb @@ -311,6 +311,43 @@ def sign_in_client(user: @user) end end + # "The same budget the web form gets" has to mean the same one, not a second + # one that happens to be the same size. Rails' rate_limit keys on the + # controller, so two controllers can never share a budget through it. + test "the web form and the app draw on one comment budget" do + token = sign_in_client + + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + begin + 3.times do |i| + post "/api/v1/plugins/acme/weather/comments", params: { body: "Through the app #{i}" }, + headers: auth(token) + assert_response :created + end + + # The browser session from signing in is still here, so the form is a + # door the same account can walk through. + 2.times do |i| + post plugin_comments_path("acme", "weather"), params: { body: "Through the form #{i}" } + assert_redirected_to plugin_path("acme", "weather") + end + + # Five all told, from both doors. The sixth is refused whichever one it + # arrives at. + post plugin_comments_path("acme", "weather"), params: { body: "One too many" } + assert_match(/slow down/i, flash[:alert]) + + post "/api/v1/plugins/acme/weather/comments", params: { body: "One too many" }, + headers: auth(token) + assert_response :too_many_requests + + assert_equal 5, @weather.comments.count + ensure + Rails.cache = original_cache + end + end + # The budget belongs to the account, not to the credential — otherwise # signing in again is the cheap way to reset it. test "signing in again does not reset the comment budget" do From 5cd497064776dedb9bf53c6773ba5184e232dd7c Mon Sep 17 00:00:00 2001 From: Jankees van Woezik Date: Sun, 30 Aug 2026 14:48:52 +0200 Subject: [PATCH 08/10] Answer which plugins an account publishes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "My plugins" was a client-side match of a guessed handle against a plugin's byline. An organisation's plugins carry the organisation's name and not the names of its members, so that got an org's work wrong in both directions — missing what you publish with other people, and claiming what a stranger happens to share a name with. GET /api/v1/me/plugins returns the manifest ids, across every namespace the account is an accepted member of. Ids rather than whole entries: the client already has the listing and only needs to know which rows are yours. That does mean one still in review is not among them, since the public listing has no row to mark. --- app/controllers/api/v1/me_controller.rb | 18 ++++++++++++++ config/routes.rb | 1 + test/integration/client_api_test.rb | 31 +++++++++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/app/controllers/api/v1/me_controller.rb b/app/controllers/api/v1/me_controller.rb index 9d05860..f75a402 100644 --- a/app/controllers/api/v1/me_controller.rb +++ b/app/controllers/api/v1/me_controller.rb @@ -30,6 +30,24 @@ def show } end + # GET /api/v1/me/plugins — what this account publishes, as manifest ids. + # + # Membership is the registry's fact and a client cannot derive it from a + # listing: an org's plugins carry the org's name, not the names of the + # people in it, so matching a handle against a byline gets an + # organisation's work wrong in both directions. Asking is the only way + # to be right. + # + # Ids rather than whole entries, because the client already has the + # listing and only needs to know which rows are yours. That does mean a + # plugin of yours still in review is not among them — it is not in the + # public listing to be marked. + def plugins + ids = Plugin.where(publisher_id: current_user.publishers.select(:id)) + .includes(:publisher).order(:name).map(&:manifest_id) + render json: { plugins: ids.sort } + end + # Signing out in the app revokes the token rather than only forgetting # it. A token the client has thrown away but the registry still honours # is exactly the credential nobody notices leaking. diff --git a/config/routes.rb b/config/routes.rb index 28071d8..5aad658 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -117,6 +117,7 @@ # the anonymous, cacheable browse API; this is everything that depends # on who is asking. Client tokens only — see ApiToken#kind. get "me", to: "me#show" + get "me/plugins", to: "me#plugins" delete "session", to: "me#destroy" scope "plugins/:publisher/:plugin", constraints: { publisher: %r{[^/]+}, plugin: %r{[^/]+} } do get "/", to: "plugins#show", as: :client_plugin diff --git a/test/integration/client_api_test.rb b/test/integration/client_api_test.rb index 6173d1f..66aff73 100644 --- a/test/integration/client_api_test.rb +++ b/test/integration/client_api_test.rb @@ -142,6 +142,37 @@ def sign_in_client(user: @user) assert_equal "no-store", response.headers["Cache-Control"] end + # An org's plugins carry the org's name, not the names of its members, so a + # client matching a handle against a byline gets this wrong in both + # directions. The registry is the only thing that knows. + test "me/plugins lists what the account publishes, across namespaces" do + token = sign_in_client + @acme.plugins.create!(name: "clock", summary: "Ticks", latest_version: "1.0.0", kinds: [ "bar-widget" ]) + + # A namespace this account has nothing to do with. + stranger = Publisher.create!(name: "someone", kind: :personal) + stranger.plugins.create!(name: "theirs", summary: "Not mine", latest_version: "1.0.0", kinds: [ "bar-widget" ]) + + get "/api/v1/me/plugins", headers: auth(token) + assert_response :success + assert_equal [ "acme.clock", "acme.weather" ], body["plugins"] + assert_equal "no-store", response.headers["Cache-Control"] + end + + test "me/plugins is empty for an account that publishes nothing" do + loner = User.create!(email_address: "loner@example.com", name: "Loner") + token = sign_in_client(user: loner) + + get "/api/v1/me/plugins", headers: auth(token) + assert_response :success + assert_equal [], body["plugins"] + end + + test "me/plugins needs a client token" do + get "/api/v1/me/plugins" + assert_response :unauthorized + end + test "signing out revokes the token rather than only forgetting it" do token = sign_in_client From 3247a849fc49f3f173139ca4f964a4863ce98263 Mon Sep 17 00:00:00 2001 From: Jankees van Woezik Date: Sun, 30 Aug 2026 15:17:41 +0200 Subject: [PATCH 09/10] Let a new rating or comment invalidate the cached directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The public read surfaces build their ETag from cache_key_with_version, which is the row's updated_at. Both totals are recomputed with update_columns, which does not touch it — so a rating landing changed the average and nothing else, every If-None-Match got a 304, and a client holding a cached listing went on showing the old number for as long as it kept asking politely. Caught by the desktop browser disagreeing with the website about the same plugin. The counters that are ALLOWED to go stale are named in ConditionalGet — downloads and views, incremented with touch: false on purpose. A rating is not one of them, and neither is a comment count. Claude-Session: https://claude.ai/code/session_01WjUWG2Z87FpQGdt5MYSDDh --- app/models/comment.rb | 7 ++++++- app/models/rating.rb | 9 ++++++++- test/integration/browse_api_test.rb | 24 ++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index f46af08..f29b4ee 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -23,7 +23,12 @@ def from_publisher? = user.member_of?(plugin.publisher) def refresh_plugin_comment_count plugin.with_lock do - plugin.update_columns(comments_count: plugin.comments.visible.count) + # updated_at too — see Rating#refresh_plugin_totals for why a total that + # moves without touching the row leaves every cached client stale. + plugin.update_columns( + comments_count: plugin.comments.visible.count, + updated_at: Time.current + ) end end diff --git a/app/models/rating.rb b/app/models/rating.rb index 63a4dc6..171dca4 100644 --- a/app/models/rating.rb +++ b/app/models/rating.rb @@ -13,9 +13,16 @@ def refresh_plugin_totals # Recompute under the plugin lock so interleaved raters can't leave the # cached totals inconsistent with the rows plugin.with_lock do + # updated_at moves with them. The public read surfaces build their ETag + # from cache_key_with_version, so totals that changed without touching + # the row answered every If-None-Match with 304 — and a client holding a + # cached listing went on showing the old average indefinitely. Counters + # that are ALLOWED to go stale (downloads, views) are named in + # ConditionalGet; a rating is not one of them. plugin.update_columns( ratings_count: plugin.ratings.count, - ratings_sum: plugin.ratings.sum(:value) + ratings_sum: plugin.ratings.sum(:value), + updated_at: Time.current ) end end diff --git a/test/integration/browse_api_test.rb b/test/integration/browse_api_test.rb index db3923e..8cf5158 100644 --- a/test/integration/browse_api_test.rb +++ b/test/integration/browse_api_test.rb @@ -32,6 +32,30 @@ def seed_filler(count) # --- directory ----------------------------------------------------------- + # A total that moves without touching the row answers every If-None-Match + # with 304, and a client holding a cached listing goes on showing the old + # number for as long as it keeps asking politely. + test "a new rating and a new comment both invalidate the directory" do + kim = User.create!(email_address: "kim@example.com", name: "Kim") + + get directory_json_path + assert_response :success + etag = response.headers["ETag"] + assert etag.present? + + get directory_json_path, headers: { "If-None-Match" => etag } + assert_response :not_modified, "nothing changed, so this should be a 304" + + @weather.ratings.create!(user: kim, value: 5) + get directory_json_path, headers: { "If-None-Match" => etag } + assert_response :success, "a rating changed the average and must invalidate" + rated_etag = response.headers["ETag"] + + @weather.comments.create!(user: kim, body: "Runs well on two monitors.") + get directory_json_path, headers: { "If-None-Match" => rated_etag } + assert_response :success, "a comment changed the count and must invalidate" + end + # The detail response layers a comments ARRAY over the same partial, so the # count needs its own name or a client parses a plugin two different ways # depending on where it found it. From fbbee4ae99f6157cc73d0bf7bf0b6241bd71943b Mon Sep 17 00:00:00 2001 From: Jankees van Woezik Date: Mon, 31 Aug 2026 22:25:42 +0200 Subject: [PATCH 10/10] Fix three things a review found in the client API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /me/plugins answered with every plugin of the account's publishers, while the comment above it promised that one still in review is not among them. The comment had the better idea. These ids exist to mark rows the client already has, so an id the directory cannot carry is one the client can never match — it can only invite it to render a row it has nothing for. Scoped to directory_visible, and written down in docs/client-api.md, which had no section for it at all. Rating something the value it already has is now a no-op. Rating's after_commit recomputes the plugin's totals and touches updated_at, and that column is what the plugin's ETag and the directory's are cut from — so an unguarded update! let one client bust a shared cache as fast as it could loop without changing a single number. A star control re-sends freely: clicking the star you already gave, or a second click landing behind the first, arrives here as the same value. And the comment budget is spent after the plugin is found rather than before. Keyed on the account, so it still has to come after authentication, but a client holding a stale id could burn all five of the hour's slots on 404s having posted nothing. Claude-Session: https://claude.ai/code/session_01Q3CDooTHJEALPb1zmgvA8W --- app/controllers/api/v1/comments_controller.rb | 7 +++-- app/controllers/api/v1/me_controller.rb | 12 +++++--- app/controllers/api/v1/ratings_controller.rb | 13 ++++++++- docs/client-api.md | 19 ++++++++++++ test/integration/client_api_test.rb | 29 +++++++++++++++++++ 5 files changed, 73 insertions(+), 7 deletions(-) diff --git a/app/controllers/api/v1/comments_controller.rb b/app/controllers/api/v1/comments_controller.rb index d3429d7..86b3df2 100644 --- a/app/controllers/api/v1/comments_controller.rb +++ b/app/controllers/api/v1/comments_controller.rb @@ -5,9 +5,12 @@ class CommentsController < BaseController include CommentRateLimit before_action :authenticate_client_token! - # After authentication, because the budget is keyed on the account. - before_action :enforce_comment_budget, only: :create + # Budget after the plugin, not before it. It is keyed on the account, so + # it has to come after authentication — but spending a slot on a request + # that then 404s means a client with a stale id can lose the hour's five + # without posting anything. before_action :load_plugin!, only: :create + before_action :enforce_comment_budget, only: :create after_action { response.headers["Cache-Control"] = "no-store" } def create diff --git a/app/controllers/api/v1/me_controller.rb b/app/controllers/api/v1/me_controller.rb index f75a402..2d811a9 100644 --- a/app/controllers/api/v1/me_controller.rb +++ b/app/controllers/api/v1/me_controller.rb @@ -39,11 +39,15 @@ def show # to be right. # # Ids rather than whole entries, because the client already has the - # listing and only needs to know which rows are yours. That does mean a - # plugin of yours still in review is not among them — it is not in the - # public listing to be marked. + # listing and only needs to know which rows are yours. Scoped to what + # the directory actually shows for the same reason: an id the listing + # cannot contain is one the client can never mark, so a plugin of yours + # still in review is not among them. Answering with it would hand the + # client ids it has nothing to match against and invite it to render a + # row it does not have. def plugins - ids = Plugin.where(publisher_id: current_user.publishers.select(:id)) + ids = Plugin.directory_visible + .where(publisher_id: current_user.publishers.select(:id)) .includes(:publisher).order(:name).map(&:manifest_id) render json: { plugins: ids.sort } end diff --git a/app/controllers/api/v1/ratings_controller.rb b/app/controllers/api/v1/ratings_controller.rb index 133f0a1..74d9ac8 100644 --- a/app/controllers/api/v1/ratings_controller.rb +++ b/app/controllers/api/v1/ratings_controller.rb @@ -39,11 +39,22 @@ def destroy # its find and its write, and a test that fakes that convincingly enough # to be worth reading has not suggested itself. The web form has the same # shape and the same exposure. + # Re-sending the rating you already have is a no-op, not a write. Rating + # has an after_commit that recomputes the plugin's totals under a lock + # and touches updated_at, which is what the plugin's and the directory's + # ETags are cut from — so an unguarded update! let one client bust a + # shared cache as fast as it could loop, without ever changing a number. + # A star control also re-sends freely: clicking the star you already + # gave, or a second click landing after the first, arrives here as the + # same value. def upsert_rating(value) rating = @plugin.ratings.find_or_initialize_by(user: current_user) + return if rating.persisted? && rating.value == value + rating.update!(value: value) rescue ActiveRecord::RecordNotUnique - @plugin.ratings.find_by!(user: current_user).update!(value: value) + existing = @plugin.ratings.find_by!(user: current_user) + existing.update!(value: value) unless existing.value == value end end end diff --git a/docs/client-api.md b/docs/client-api.md index 6e8a5e1..72b1ce0 100644 --- a/docs/client-api.md +++ b/docs/client-api.md @@ -121,6 +121,25 @@ failed write. memberships only. The personal one is the account's own handle — a better answer than anything a client can infer from what is installed locally. +## `GET /api/v1/me/plugins` + +Which plugins this account publishes, as manifest ids. + +```json +{ "plugins": ["acme.weather", "kimrivera.clock"] } +``` + +A client cannot work this out from a listing. An organisation's plugins carry +the organisation's name and not the names of its members, so matching a handle +against a byline gets an org's work wrong in both directions — it claims other +people's plugins for whoever shares a handle with the namespace, and disowns +the ones published under a name you share with colleagues. Asking is the only +way to be right. + +Scoped to what the directory shows, so a plugin still in review is not in the +list. The ids exist to mark rows the client already has; one the listing cannot +contain is one it could never mark. + ## `DELETE /api/v1/session` Signing out. Revokes **this** token and answers `204`; other devices keep diff --git a/test/integration/client_api_test.rb b/test/integration/client_api_test.rb index 66aff73..da334c4 100644 --- a/test/integration/client_api_test.rb +++ b/test/integration/client_api_test.rb @@ -159,6 +159,19 @@ def sign_in_client(user: @user) assert_equal "no-store", response.headers["Cache-Control"] end + # The ids exist to mark rows the client already has. One the directory does + # not carry is one it can never mark, so answering with it only invites the + # client to render a row it has nothing for. + test "me/plugins leaves out a plugin the directory does not show" do + token = sign_in_client + @acme.plugins.create!(name: "unreleased", summary: "Still in review", + latest_version: nil, kinds: [ "bar-widget" ]) + + get "/api/v1/me/plugins", headers: auth(token) + assert_response :success + assert_equal [ "acme.weather" ], body["plugins"] + end + test "me/plugins is empty for an account that publishes nothing" do loner = User.create!(email_address: "loner@example.com", name: "Loner") token = sign_in_client(user: loner) @@ -218,6 +231,22 @@ def sign_in_client(user: @user) # --- rating --------------------------------------------------------------- + # The plugin's updated_at is what its ETag and the directory's are cut from, + # so a write that changes no number must not invalidate a shared cache. A + # star control re-sends the value it already has freely. + test "rating a plugin the value it already has changes nothing" do + token = sign_in_client + put "/api/v1/plugins/acme/weather/rating", params: { value: 4 }, headers: auth(token) + assert_response :success + before = @weather.reload.updated_at + + put "/api/v1/plugins/acme/weather/rating", params: { value: 4 }, headers: auth(token) + assert_response :success + assert_equal 4, body["rating"]["mine"] + assert_equal before, @weather.reload.updated_at, + "a no-op rating touched the plugin and busted its cache" + end + test "rating a plugin, moving the rating, and clearing it" do token = sign_in_client