diff --git a/CHANGELOG.md b/CHANGELOG.md index 2904278..bd69e9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## [Unreleased] +- Fix saving a shopkeeper in madmin failing with 500 (`PG::UniqueViolation` on `index_shopkeepers_on_reset_password_token` or `index_shopkeepers_on_confirmation_token`). The edit form sends empty token fields as `""`, so a second shopkeeper with no reset or confirmation token collided with the first. madmin's shopkeeper form no longer shows `reset_password_token` and `confirmation_token` (read only on the show page), and a migration clears the `""` already stored. An earlier version of this fix normalized blank tokens to NULL in `Shopkeeper`; that also rewrote finder values, so `where(reset_password_token: "")` matched every shopkeeper without a token, and it was removed. Ported from myturntagapi, where it hit production (#158) - API change: `Shop#time_zone` and `Shopkeeper#time_zone` must be a Rails time zone name (`ActiveSupport::TimeZone.all.map(&:name)`, e.g. `Tokyo`, `London`); anything else, IANA ids such as `Asia/Tokyo` included, gets 422 "Time zone is not included in the list" from `PATCH /shops/:id`, `POST /shops`, sign-up and `PATCH /shopkeeper_auth` instead of being stored. `test/config/time_zones_test.rb` pins the list against `test/fixtures/files/rails_time_zones.tsv`, which the iOS and Android apps embed for their time zone pickers (#155) - Add optional Sentry error monitoring (`sentry-ruby` / `sentry-rails` 7.1). Off unless `SENTRY_DSN` is set (a Kamal secret read from `NATIVEAPPTEMPLATEAPI_SENTRY_DSN`, optional); sends only from production, only unhandled errors (no 4xx, no tracing, profiling or breadcrumbs), with the shopkeeper's ID as the only user data, the account ID as a tag, devise_token_auth headers dropped, email addresses masked and `filter_parameters` applied. The README's "Error monitoring (Sentry)" covers turning it on, what is sent and the recommended Sentry privacy settings. New tests: `test/config/sentry_test.rb`, `test/integration/sentry_context_test.rb` (#154) - API change: tell the apps when the organization in the URL is gone. An `//api/...` request for a deleted account now gets `404 {"code":404,"error_code":"account_not_found","error_message":...}` JSON (was a `302` to `/`), and a signed-in shopkeeper who is no longer a member gets `403 {"code":403,"error_code":"not_account_member","error_message":...}` (was Pundit's generic 401, which the apps treated as a sign-out). Messages are in English and Japanese and follow the usual locale rules. Pundit denials inside an organization the shopkeeper belongs to stay 401; non-API paths still redirect (#153) diff --git a/app/madmin/resources/shopkeeper_resource.rb b/app/madmin/resources/shopkeeper_resource.rb index d049b8d..4885a50 100644 --- a/app/madmin/resources/shopkeeper_resource.rb +++ b/app/madmin/resources/shopkeeper_resource.rb @@ -4,11 +4,13 @@ class ShopkeeperResource < Madmin::Resource attribute :provider attribute :uid attribute :encrypted_password - attribute :reset_password_token + # Read only: the form would send "" for an empty token, which breaks its unique index + attribute :reset_password_token, form: false attribute :reset_password_sent_at attribute :allow_password_change attribute :remember_created_at - attribute :confirmation_token + # Read only: the form would send "" for an empty token, which breaks its unique index + attribute :confirmation_token, form: false attribute :confirmed_at attribute :confirmation_sent_at attribute :unconfirmed_email diff --git a/app/models/shopkeeper.rb b/app/models/shopkeeper.rb index 997fd67..9e8be5b 100644 --- a/app/models/shopkeeper.rb +++ b/app/models/shopkeeper.rb @@ -17,6 +17,11 @@ class Shopkeeper < ApplicationRecord has_many :application_push_devices, as: :owner, class_name: "ApplicationPushDevice", dependent: :destroy has_many :notifications, as: :recipient, dependent: :destroy, class_name: "Noticed::Notification" + # No `normalizes` on the token columns: it also rewrites finder values, and + # turning "" into nil makes where(reset_password_token: "") match every + # shopkeeper without a token. madmin keeps the tokens out of its edit form + # instead (ShopkeeperResource), so it never writes "" into their unique indexes. + attribute :token, :string attribute :client, :string attribute :expiry, :string diff --git a/db/migrate/20261017000000_nullify_blank_shopkeeper_tokens.rb b/db/migrate/20261017000000_nullify_blank_shopkeeper_tokens.rb new file mode 100644 index 0000000..e0ff7f0 --- /dev/null +++ b/db/migrate/20261017000000_nullify_blank_shopkeeper_tokens.rb @@ -0,0 +1,12 @@ +# Saving a shopkeeper in madmin stored "" in empty token fields; a second "" +# then broke the unique indexes. Shopkeeper now normalizes blanks to NULL, and +# this clears the "" already stored. +class NullifyBlankShopkeeperTokens < ActiveRecord::Migration[8.1] + def up + execute "UPDATE shopkeepers SET reset_password_token = NULL WHERE reset_password_token = ''" + execute "UPDATE shopkeepers SET confirmation_token = NULL WHERE confirmation_token = ''" + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index e415011..92f7511 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_10_04_010000) do +ActiveRecord::Schema[8.1].define(version: 2026_10_17_000000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" diff --git a/test/integration/madmin_test.rb b/test/integration/madmin_test.rb index 302f94c..b70d74f 100644 --- a/test/integration/madmin_test.rb +++ b/test/integration/madmin_test.rb @@ -61,6 +61,30 @@ class MadminTest < ActionDispatch::IntegrationTest assert_equal "Renamed by admin", @shop.reload.name end + # The edit form sends every field it shows, not just the one the admin changed, so + # empty token fields arrive as "". Ported from myturntagapi, where in production + # (2026-10-17) saving a shopkeeper whose reset_password_token was nil failed with + # PG::UniqueViolation on index_shopkeepers_on_reset_password_token, + # "Key (reset_password_token)=() already exists", because an earlier save had + # stored "" on another shopkeeper. + test "saves the shopkeeper edit form as the browser submits it" do + shopkeepers(:two).update_columns(reset_password_token: "", confirmation_token: "") + @shopkeeper.update_columns(reset_password_token: nil, confirmation_token: nil) + sign_in_admin + get ShopkeeperResource.edit_path(@shopkeeper) + assert_response :success + + form = Nokogiri::HTML(response.body).at_css("form[action$='/shopkeepers/#{@shopkeeper.id}']") + fields = form_fields(form).map do |name, value| + (name == "shopkeeper[name]") ? [name, "Renamed by admin"] : [name, value] + end + + patch madmin_shopkeeper_path(@shopkeeper), params: URI.encode_www_form(fields), + headers: {"CONTENT_TYPE" => "application/x-www-form-urlencoded"} + assert_redirected_to madmin_shopkeeper_path(@shopkeeper) + assert_equal "Renamed by admin", @shopkeeper.reload.name + end + test "blobs and variant records hide write actions" do sign_in_admin @@ -99,6 +123,23 @@ class MadminTest < ActionDispatch::IntegrationTest private + # [name, value] pairs a browser sends for the form: filled inputs and textareas, + # checked boxes and the selected options of each select + def form_fields(form) + form.css("input, textarea, select").flat_map do |field| + name = field["name"] + next [] if name.blank? || field["disabled"] || %w[submit button].include?(field["type"]) + + case field.name + when "select" then field.css("option[selected]").map { |option| [name, option["value"] || option.text] } + when "textarea" then [[name, field.text]] + else + next [] if %w[checkbox radio].include?(field["type"]) && !field["checked"] + [[name, field["value"].to_s]] + end + end + end + def sign_in_admin post admin_session_path, params: {email: @admin_user.email, password: "password"} end diff --git a/test/models/shopkeeper_test.rb b/test/models/shopkeeper_test.rb index 30ed0f5..b06ba63 100644 --- a/test/models/shopkeeper_test.rb +++ b/test/models/shopkeeper_test.rb @@ -315,4 +315,17 @@ class ShopkeeperTest < ActiveSupport::TestCase shopkeeper.name = "Renamed" assert shopkeeper.valid? end + + # A token column must never be normalized: normalization also runs on finder values, + # so turning "" into nil would make where(reset_password_token: "") match every + # shopkeeper with no token (WHERE reset_password_token IS NULL) + test "a blank token finds no shopkeeper" do + shopkeepers(:one).update_columns(reset_password_token: nil, confirmation_token: nil) + + # where, as Devise's lookups do (orm_adapter's find_first); find_by binds nil as + # "= NULL" instead and would match nothing either way + assert_not Shopkeeper.where(reset_password_token: "").exists? + assert_not Shopkeeper.where(confirmation_token: "").exists? + assert_nil Shopkeeper.with_reset_password_token("") + end end