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..86b3df2 --- /dev/null +++ b/app/controllers/api/v1/comments_controller.rb @@ -0,0 +1,45 @@ +module Api + module V1 + class CommentsController < BaseController + include PluginScoped + include CommentRateLimit + + before_action :authenticate_client_token! + # 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 + 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 + + 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/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..2d811a9 --- /dev/null +++ b/app/controllers/api/v1/me_controller.rb @@ -0,0 +1,64 @@ +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 + + # 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. 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.directory_visible + .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. + 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..74d9ac8 --- /dev/null +++ b/app/controllers/api/v1/ratings_controller.rb @@ -0,0 +1,61 @@ +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 + + upsert_rating(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 + + 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. + # 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 + existing = @plugin.ratings.find_by!(user: current_user) + existing.update!(value: value) unless existing.value == value + 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/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/api/v1/plugin_scoped.rb b/app/controllers/concerns/api/v1/plugin_scoped.rb new file mode 100644 index 0000000..5726b5e --- /dev/null +++ b/app/controllers/concerns/api/v1/plugin_scoped.rb @@ -0,0 +1,67 @@ +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, + # 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, + 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/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/app/controllers/device_controller.rb b/app/controllers/device_controller.rb index 295f160..9da74b7 100644 --- a/app/controllers/device_controller.rb +++ b/app/controllers/device_controller.rb @@ -1,7 +1,16 @@ -# 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_no_sensitive_cooldown, only: :approve + before_action :require_recent_second_factor, only: :approve, if: :minting_publish_token? + before_action :require_no_sensitive_cooldown, only: :approve, unless: :denying? def show @user_code = params[:code] @@ -29,7 +38,45 @@ 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 + + # 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. 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 + # 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 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/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/comment.rb b/app/models/comment.rb index b8459f7..f29b4ee 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,29 @@ 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 + # 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 + def hide!(actor:) update!(hidden_at: Time.current) AuditEvent.record!(actor:, action: "comment.hide", subject: self, 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/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/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/app/views/plugins/_plugin.json.jbuilder b/app/views/plugins/_plugin.json.jbuilder index d0df6c6..6df2200 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 @@ -22,6 +30,14 @@ json.rating do json.average plugin.average_rating json.count plugin.ratings_count end +# 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/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/config/routes.rb b/config/routes.rb index 1ff50cf..5aad658 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -111,6 +111,21 @@ 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" + 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 + 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/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 c2567d2..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_25_120003) 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 @@ -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/browse-api.md b/docs/browse-api.md index ca82109..fd261bc 100644 --- a/docs/browse-api.md +++ b/docs/browse-api.md @@ -78,6 +78,13 @@ category:system curated category } ``` +`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 decision and the list changes without warning. @@ -128,6 +135,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", @@ -141,6 +150,7 @@ One plugin, with everything the web page renders. "downloads": 500, "views": 12, "rating": { "average": 4.5, "count": 10 }, + "comments_count": 3, "repository": { "url": "https://github.com/acme/weather", "label": "GitHub", "stars": 42, @@ -171,6 +181,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/docs/client-api.md b/docs/client-api.md new file mode 100644 index 0000000..72b1ce0 --- /dev/null +++ b/docs/client-api.md @@ -0,0 +1,214 @@ +# 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. 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 + +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. + +## `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 +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", + "comments_count": 3, + "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. +- `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//` + +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 **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/` + +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/browse_api_test.rb b/test/integration/browse_api_test.rb index e74278e..8cf5158 100644 --- a/test/integration/browse_api_test.rb +++ b/test/integration/browse_api_test.rb @@ -32,6 +32,71 @@ 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. + 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_count"] + 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 + @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 @@ -45,8 +110,15 @@ 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_count"] 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" diff --git a/test/integration/client_api_test.rb b/test/integration/client_api_test.rb new file mode 100644 index 0000000..da334c4 --- /dev/null +++ b/test/integration/client_api_test.rb @@ -0,0 +1,457 @@ +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 + + # 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 + + # 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 + 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 + + # 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 + + # 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) + + 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 + + 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 --------------------------------------------------------------- + + # 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 + + 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 neither in the thread nor in the count" do + token = sign_in_client + @weather.comments.create!(user: @user, body: "Visible comment") + 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 + # 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 + + # "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 + 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 + 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