From 14490d3a1a0154ad4b12b23cb09b34fedc153cf4 Mon Sep 17 00:00:00 2001 From: dadachi Date: Sat, 17 Oct 2026 08:06:33 +0900 Subject: [PATCH 1/2] Store blank shopkeeper tokens as NULL so madmin can save shopkeepers Ported from myturntagapi, where saving a shopkeeper in madmin returned 500 in production: PG::UniqueViolation on index_shopkeepers_on_reset_password_token, "Key (reset_password_token)=() already exists". The edit form sends every field, so an empty token arrives as "", and once one shopkeeper had "" stored, the next one with no token collided with it. The template has the same form fields and unique indexes. Shopkeeper normalizes blank reset_password_token and confirmation_token (both unique) to NULL, and a migration clears the "" already stored. The madmin test submits the edit form with every field, as a browser does, while another shopkeeper holds "" tokens. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y3Tt4YrVgQRxh5MCpiiric --- CHANGELOG.md | 1 + app/models/shopkeeper.rb | 4 ++ ...7000000_nullify_blank_shopkeeper_tokens.rb | 12 ++++++ db/schema.rb | 2 +- test/integration/madmin_test.rb | 41 +++++++++++++++++++ 5 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20261017000000_nullify_blank_shopkeeper_tokens.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 2904278..723b983 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. `Shopkeeper` now stores blank `reset_password_token` and `confirmation_token` as NULL, and a migration clears the `""` already stored. Ported from myturntagapi, where it hit production - 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/models/shopkeeper.rb b/app/models/shopkeeper.rb index 997fd67..235991f 100644 --- a/app/models/shopkeeper.rb +++ b/app/models/shopkeeper.rb @@ -17,6 +17,10 @@ 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" + # Unique columns: a blank token must be NULL, or a second blank one (madmin's + # edit form sends "" for empty fields) violates the unique index + normalizes :reset_password_token, :confirmation_token, with: ->(token) { token.presence } + 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 From 3aefaabd50d7365e2fa44ee5db6be38246f17b43 Mon Sep 17 00:00:00 2001 From: dadachi Date: Sat, 17 Oct 2026 08:13:29 +0900 Subject: [PATCH 2/2] Keep madmin from writing blank tokens instead of normalizing them The previous commit normalized blank reset_password_token and confirmation_token to NULL. Normalization also rewrites finder values, so where(reset_password_token: "") became WHERE reset_password_token IS NULL and matched every shopkeeper without a token. Ported from myturntagapi #219. Drop the normalization and keep the two tokens out of madmin's shopkeeper form (form: false), the only writer that sent "". The migration that clears stored "" values stays. ShopkeeperTest checks that a blank token finds no shopkeeper (it failed with the normalization); the madmin form test still fails with the token fields back in the form. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y3Tt4YrVgQRxh5MCpiiric --- CHANGELOG.md | 2 +- app/madmin/resources/shopkeeper_resource.rb | 6 ++++-- app/models/shopkeeper.rb | 7 ++++--- test/models/shopkeeper_test.rb | 13 +++++++++++++ 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 723b983..bd69e9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +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. `Shopkeeper` now stores blank `reset_password_token` and `confirmation_token` as NULL, and a migration clears the `""` already stored. Ported from myturntagapi, where it hit production +- 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 235991f..9e8be5b 100644 --- a/app/models/shopkeeper.rb +++ b/app/models/shopkeeper.rb @@ -17,9 +17,10 @@ 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" - # Unique columns: a blank token must be NULL, or a second blank one (madmin's - # edit form sends "" for empty fields) violates the unique index - normalizes :reset_password_token, :confirmation_token, with: ->(token) { token.presence } + # 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 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