Answer deleted or left organizations with distinct JSON errors

AGENTS.md

@@ -65,6 +65,7 @@ bin/rails dbconsole           # Database console
 - **Authorization**: Pundit policies for resource-level permissions
 - **Multi-tenancy**: acts_as_tenant for complete data isolation between accounts
 - **RBAC**: Role and Permission models for fine-grained access control
+- **Organization errors**: an `/<account uuid>/api/...` request for a deleted account gets `404 {code, error_code: "account_not_found", error_message}` from `AccountMiddleware` (via `Api::V1::Shopkeeper::AccountNotFoundController`, JSON whatever the request headers); a signed-in shopkeeper who is not a member gets `403 {code, error_code: "not_account_member", error_message}` from `Api::V1::Shopkeeper::BaseController`. The apps switch to the personal organization on these instead of signing out. Pundit denials stay `401 {code, error_message}`; devise_token_auth's invalid-token 401 is `{errors: [...]}`. Non-API paths with an unknown account id still redirect to `/`.
 
 ### Key Models & Relationships
 - `Account` - Top-level tenant/organization
@@ -103,7 +104,7 @@ Cross-platform push via the `noticed` (v3) and `action_push_native` gems. APNs f
   - `json_response` for parsing JSON API responses
   - `create_new_auth_token` for generating auth headers (Devise Token Auth)
   - Fixtures in test/fixtures/ and seed data in db/fixtures/test/
-- Run tests: `bin/rails test` (584 tests, 1458 assertions)
+- Run tests: `bin/rails test` (602 tests, 1514 assertions)
 
 ### Internationalization
 - Responses are in English (default) or Japanese, picked by the `SetLocale` concern (included in `ApplicationController` and `NonApiApplicationController`). It wraps `process_action`, not `around_action`, so `rescue_from` handlers and `rate_limit` responses are localized too.

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## [Unreleased]
 
+- API change: tell the apps when the organization in the URL is gone. An `/<account uuid>/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)
 - Call a shop 店舗 rather than ショップ in the Japanese messages (model name, item tag's shop attribute, the shop limit error, the sample shop description), matching 店舗名
 - API change: add a Japanese locale. A signed-in shopkeeper gets the language in `shopkeepers.locale`, which sign-up now sets from a `locale` param (falling back to `Accept-Language`) and `PATCH /shopkeeper_auth` can change (`en` or `ja`, else 422). Other requests follow the `Accept-Language` header (English by default) via the new `SetLocale` concern. Mails to a shopkeeper use their `locale`; an invitation to an email with no account uses the inviter's. Translations are in `config/locales/ja.yml`, `rails.ja.yml` and `devise.ja.yml`, with no extra gems. Hard-coded English in the error pages, `ErrorsController`, the sample item tag and the mail greetings moved to locale keys. `test/i18n/locale_files_test.rb` fails on any untranslated key. Two English messages changed: an unknown `current_platform` now gets the same "Your input data is wrong." as a blank one, and a duplicate member gets "User is already a member of this organization" instead of "has already been taken". Fix `PATCH /shopkeeper_auth` returning 422 unless `email` was sent: devise_token_auth checked the params before `name`, `time_zone` and `locale` were permitted (#150)
 - Fix a failed sign-in saving the `source` header as `current_platform`: anyone knowing an email could set it to any string, after which that shopkeeper's profile updates returned 422. Only successful sign-ins write it, and only `ios` or `android`. Fix resending an invitation whose inviter was deleted (the mail job raised on `nil.name`); the mail names the app instead. Rename `.erb-lint.yml` to `.erb_lint.yml` and drop a removed RuboCop cop from `.rubocop.yml` (#149)
@@ -26,7 +27,7 @@
 - API change: members and invitations must have at least one role (422 "Roles can't be blank"). A role-less member made `GET /permissions` raise (#121)
 - API change: password reset and confirmation `redirect_url` must be on the API's own host. Other hosts get 422 for resets and fall back to the default page for confirmations (#120)
 - Harden admin sign-in: rate limit by IP and email, `reset_session` on sign-in and sign-out, and Madmin now rejects a deleted admin's session (#119)
-- AGENTS.md: correct the CORS and test-worker notes, and keep the test count current (#118, #127, #132, #147, #149, #150)
+- AGENTS.md: correct the CORS and test-worker notes, and keep the test count current (#118, #127, #132, #147, #149, #150, #153)
 - Clear finished Solid Queue jobs every hour via `config/recurring.yml` (#117)
 - API change: `item_tags#update` no longer accepts `state`. Completion goes through the `complete`/`idle` events, and an unknown state no longer returns 500 (#116)
 - API change: API errors are always JSON `{code, error_message}`, including missing records (404), missing parameters (400), unknown API routes and 500s; before, they were HTML unless the request sent a JSON Content-Type (#115)

app/controllers/api/v1/shopkeeper/account_not_found_controller.rb

@@ -0,0 +1,16 @@
+# Answers an /<account uuid>/api/... request whose organization no longer
+# exists. AccountMiddleware dispatches here directly (there is no route), so
+# the client gets JSON whatever its Accept or Content-Type, and can tell this
+# apart from other 404s by error_code and switch to its personal organization.
+#
+# The token is checked only to pick the language: a signed-in shopkeeper gets
+# their saved locale, anyone else Accept-Language (SetLocale).
+class Api::V1::Shopkeeper::AccountNotFoundController < ApplicationController
+  include DeviseTokenAuth::Concerns::SetUserByToken
+
+  def show
+    use_locale_of(current_shopkeeper)
+    render json: {code: 404, error_code: "account_not_found", error_message: I18n.t("api.shopkeeper.account_not_found")},
+      status: :not_found
+  end
+end

app/controllers/api/v1/shopkeeper/base_controller.rb

@@ -7,6 +7,7 @@ class Api::V1::Shopkeeper::BaseController < ApplicationController
 
   before_action :authenticate_shopkeeper!
   before_action -> { use_locale_of(current_shopkeeper) }
+  before_action :require_account_membership
   after_action :verify_authorized
 
   rescue_from ActiveRecord::RecordNotFound, with: :record_not_found
@@ -36,6 +37,16 @@ def render_error(code:, message:, status:)
     render json: {code: code, error_message: message}, status: status
   end
 
+  # The organization in the URL exists, but the shopkeeper is no longer one of
+  # its members (removed, or left). Not a Pundit denial: the app switches to
+  # the personal organization on this error_code instead of signing out.
+  def require_account_membership
+    return if current_account.nil? || current_accounts_shopkeeper
+
+    render json: {code: 403, error_code: "not_account_member", error_message: I18n.t("api.shopkeeper.not_account_member")},
+      status: :forbidden
+  end
+
   def user_not_authorized
     render_error(code: 401, message: I18n.t("unauthorized"), status: :unauthorized)
   end

config/locales/en.yml

@@ -65,6 +65,8 @@ en:
 
   api:
     shopkeeper:
+      account_not_found: "This organization no longer exists."
+      not_account_member: "You are no longer a member of this organization."
       accounts:
         personal:
           cannot_delete: "You cannot delete your personal account."

config/locales/ja.yml

@@ -41,6 +41,8 @@ ja:
 
   api:
     shopkeeper:
+      account_not_found: "この組織は削除されました。"
+      not_account_member: "この組織のメンバーではなくなりました。"
       accounts:
         personal:
           cannot_delete: "個人用の組織は削除できません。"

lib/middleware/account_middleware.rb

@@ -20,6 +20,10 @@ def call(env)
     if UUID_MATCHER.match?(account_id)
       if (account = Account.find_by(id: account_id))
         Current.account = account
+      elsif request_path.to_s.start_with?("api/")
+        # The organization was deleted: answer in JSON so the app can switch
+        # to the shopkeeper's personal organization
+        return Api::V1::Shopkeeper::AccountNotFoundController.action(:show).call(env)
       else
         return [302, {"Location" => "/"}, []]
       end

test/integration/account_membership_errors_test.rb

@@ -0,0 +1,187 @@
+require "test_helper"
+
+# A client keeps the current organization's id in every API path
+# (/<account uuid>/api/...). When that organization is deleted, or the
+# shopkeeper is removed from it, the client must be able to tell those cases
+# apart from other errors, so it can switch to the personal organization
+# instead of signing the shopkeeper out:
+#
+#   deleted organization            -> 404 {code, error_code: "account_not_found", error_message}
+#   signed in, no longer a member   -> 403 {code, error_code: "not_account_member", error_message}
+#
+# Everything else is unchanged: Pundit denials inside an organization the
+# shopkeeper belongs to stay 401 with error_message, and devise_token_auth's
+# invalid-token 401 keeps its {"errors": [...]} body.
+class AccountMembershipErrorsTest < ActionDispatch::IntegrationTest
+  # Account ids are Postgres gen_random_uuid() values; this one never exists
+  MISSING_ACCOUNT_ID = "00000000-0000-0000-0000-000000000000"
+
+  # The wording of the contract (org-membership-contract.md)
+  ACCOUNT_NOT_FOUND = {
+    en: "This organization no longer exists.",
+    ja: "この組織は削除されました。"
+  }.freeze
+  NOT_ACCOUNT_MEMBER = {
+    en: "You are no longer a member of this organization.",
+    ja: "この組織のメンバーではなくなりました。"
+  }.freeze
+
+  setup do
+    @shopkeeper = shopkeepers(:one)
+    @shopkeeper.create_default_account
+    @owner = shopkeepers(:two)
+    @owner.create_default_account
+    # A team organization the shopkeeper was removed from (or never joined)
+    @team = Account.create!(name: "Team", owner: @owner, personal: false)
+    @team.accounts_shopkeepers.create!(shopkeeper: @owner, admin: true)
+  end
+
+  # --- Deleted organization -------------------------------------------------
+
+  test "a deleted organization answers 404 account_not_found for a signed-in GET without Content-Type" do
+    get "/#{MISSING_ACCOUNT_ID}/api/v1/shopkeeper/permissions", headers: @shopkeeper.create_new_auth_token
+
+    assert_account_error 404, "account_not_found", ACCOUNT_NOT_FOUND[:en]
+  end
+
+  test "a deleted organization answers 404 account_not_found for a JSON request" do
+    patch "/#{MISSING_ACCOUNT_ID}/api/v1/shopkeeper/shops/#{MISSING_ACCOUNT_ID}",
+      params: {shop: {name: "x"}}.to_json,
+      headers: @shopkeeper.create_new_auth_token.merge("Content-Type" => "application/json", "Accept" => "application/json")
+
+    assert_account_error 404, "account_not_found", ACCOUNT_NOT_FOUND[:en]
+  end
+
+  # Android sends only the JSON:API media type
+  test "a deleted organization answers 404 account_not_found for Accept application/vnd.api+json" do
+    get "/#{MISSING_ACCOUNT_ID}/api/v1/shopkeeper/shops",
+      headers: @shopkeeper.create_new_auth_token.merge("Accept" => "application/vnd.api+json")
+
+    assert_account_error 404, "account_not_found", ACCOUNT_NOT_FOUND[:en]
+  end
+
+  test "an organization deleted after the client stored it answers 404 account_not_found" do
+    team_id = @team.id
+    ActsAsTenant.without_tenant { @team.destroy! }
+
+    get "/#{team_id}/api/v1/shopkeeper/shops", headers: @owner.create_new_auth_token
+
+    assert_account_error 404, "account_not_found", ACCOUNT_NOT_FOUND[:en]
+  end
+
+  test "a deleted organization answers 404 account_not_found even without a valid token" do
+    get "/#{MISSING_ACCOUNT_ID}/api/v1/shopkeeper/shops"
+
+    assert_account_error 404, "account_not_found", ACCOUNT_NOT_FOUND[:en]
+  end
+
+  test "account_not_found uses the signed-in shopkeeper's saved locale" do
+    @shopkeeper.update!(locale: "ja")
+
+    get "/#{MISSING_ACCOUNT_ID}/api/v1/shopkeeper/shops",
+      headers: @shopkeeper.create_new_auth_token.merge("Accept-Language" => "en")
+
+    assert_account_error 404, "account_not_found", ACCOUNT_NOT_FOUND[:ja]
+  end
+
+  test "account_not_found uses Accept-Language without a signed-in shopkeeper" do
+    get "/#{MISSING_ACCOUNT_ID}/api/v1/shopkeeper/shops", headers: {"Accept-Language" => "ja-JP"}
+
+    assert_account_error 404, "account_not_found", ACCOUNT_NOT_FOUND[:ja]
+  end
+
+  test "a deleted organization in a non-API path still redirects to the root" do
+    get "/#{MISSING_ACCOUNT_ID}/shopkeeper_auth/confirmation_result"
+
+    assert_response :found
+    assert_equal "/", response.headers["Location"]
+  end
+
+  # --- Signed in, not a member ---------------------------------------------
+
+  test "permissions in an organization the shopkeeper left answers 403 not_account_member" do
+    get "/#{@team.id}/api/v1/shopkeeper/permissions", headers: @shopkeeper.create_new_auth_token
+
+    assert_account_error 403, "not_account_member", NOT_ACCOUNT_MEMBER[:en]
+  end
+
+  test "shops in an organization the shopkeeper left answers 403 not_account_member" do
+    get "/#{@team.id}/api/v1/shopkeeper/shops", headers: @shopkeeper.create_new_auth_token
+
+    assert_account_error 403, "not_account_member", NOT_ACCOUNT_MEMBER[:en]
+    assert_nil response.parsed_body["data"]
+  end
+
+  test "the accounts list under an organization the shopkeeper left answers 403 not_account_member" do
+    get "/#{@team.id}/api/v1/shopkeeper/accounts", headers: @shopkeeper.create_new_auth_token
+
+    assert_account_error 403, "not_account_member", NOT_ACCOUNT_MEMBER[:en]
+  end
+
+  test "an invitation looked up under an organization the shopkeeper left answers 403 not_account_member" do
+    invitation = AccountsInvitation.create!(account: @owner.accounts.personal.first,
+      name: "Invited", email: @shopkeeper.email, member: true)
+
+    get "/#{@team.id}/api/v1/shopkeeper/accounts_invitations/#{invitation.token}",
+      headers: @shopkeeper.create_new_auth_token
+
+    assert_account_error 403, "not_account_member", NOT_ACCOUNT_MEMBER[:en]
+  end
+
+  test "not_account_member uses the shopkeeper's saved locale over Accept-Language" do
+    @shopkeeper.update!(locale: "ja")
+
+    get "/#{@team.id}/api/v1/shopkeeper/shops",
+      headers: @shopkeeper.create_new_auth_token.merge("Accept-Language" => "en")
+
+    assert_account_error 403, "not_account_member", NOT_ACCOUNT_MEMBER[:ja]
+  end
+
+  test "not_account_member is English for a shopkeeper whose saved locale is en" do
+    get "/#{@team.id}/api/v1/shopkeeper/shops",
+      headers: @shopkeeper.create_new_auth_token.merge("Accept-Language" => "ja")
+
+    assert_account_error 403, "not_account_member", NOT_ACCOUNT_MEMBER[:en]
+  end
+
+  # --- Unchanged ------------------------------------------------------------
+
+  test "a real Pundit denial inside an organization the shopkeeper belongs to stays 401" do
+    # AccountPolicy#update? requires admin; the shopkeeper is a plain member
+    @team.accounts_shopkeepers.create!(shopkeeper: @shopkeeper, admin: false, member: true)
+
+    patch "/#{@team.id}/api/v1/shopkeeper/accounts/#{@team.id}",
+      params: {account: {name: "Renamed"}},
+      headers: @shopkeeper.create_new_auth_token
+
+    assert_response :unauthorized
+    assert_equal({"code" => 401, "error_message" => I18n.t("unauthorized", locale: :en)}, response.parsed_body)
+  end
+
+  test "a member still reaches the organization" do
+    @team.accounts_shopkeepers.create!(shopkeeper: @shopkeeper, admin: false, member: true)
+
+    get "/#{@team.id}/api/v1/shopkeeper/shops", headers: @shopkeeper.create_new_auth_token
+
+    assert_response :success
+  end
+
+  test "an invalid token in an existing organization keeps devise_token_auth's 401" do
+    headers = @shopkeeper.create_new_auth_token.merge("access-token" => "wrong")
+
+    get "/#{@team.id}/api/v1/shopkeeper/shops", headers: headers
+
+    assert_response :unauthorized
+    assert response.parsed_body["errors"].present?
+    assert_nil response.parsed_body["error_code"]
+    assert_nil response.parsed_body["error_message"]
+  end
+
+  private
+
+  def assert_account_error(status, error_code, message)
+    assert_response status
+    assert_equal "application/json", response.media_type
+    assert_equal({"code" => status, "error_code" => error_code, "error_message" => message}, response.parsed_body)
+  end
+end

test/integration/tenant_and_admin_routes_test.rb

@@ -2,8 +2,9 @@
 
 class TenantAndAdminRoutesTest < ActionDispatch::IntegrationTest
   # AccountMiddleware selects any existing account from the URL without
-  # checking membership; Pundit then refuses a shopkeeper who isn't a member.
-  test "another tenant's account id in the URL gets 401 and no data" do
+  # checking membership; Api::V1::Shopkeeper::BaseController then refuses a
+  # shopkeeper who isn't a member (403 not_account_member).
+  test "another tenant's account id in the URL gets 403 and no data" do
     outsider = shopkeepers(:one)
     outsider.create_default_account
     owner = shopkeepers(:two)
@@ -12,8 +13,9 @@ class TenantAndAdminRoutesTest < ActionDispatch::IntegrationTest
 
     get "/#{other_account.id}/api/v1/shopkeeper/shops", headers: outsider.create_new_auth_token
 
-    assert_response :unauthorized
-    assert_equal 401, response.parsed_body["code"]
+    assert_response :forbidden
+    assert_equal 403, response.parsed_body["code"]
+    assert_equal "not_account_member", response.parsed_body["error_code"]
     assert_nil response.parsed_body["data"]
   end
 

test/lib/account_middleware_test.rb

@@ -37,8 +37,18 @@ class AccountMiddlewareTest < ActiveSupport::TestCase
     assert_equal "/api/v1/shopkeeper/shops", @seen[:path_info]
   end
 
-  test "an unknown account id redirects to the root" do
-    status, headers, = call("/00000000-0000-0000-0000-000000000000/api/v1/shopkeeper/shops")
+  # The organization was deleted: the apps need JSON to switch organizations
+  test "an unknown account id in an API path answers 404 account_not_found JSON" do
+    status, headers, body = call("/00000000-0000-0000-0000-000000000000/api/v1/shopkeeper/shops")
+
+    assert_equal 404, status
+    assert_match %r{\Aapplication/json}, headers["content-type"]
+    assert_equal "account_not_found", JSON.parse(body.each.to_a.join)["error_code"]
+    assert_empty @seen, "the app must not be called"
+  end
+
+  test "an unknown account id outside the API redirects to the root" do
+    status, headers, = call("/00000000-0000-0000-0000-000000000000/shopkeeper_auth/confirmation_result")
 
     assert_equal 302, status
     assert_equal "/", headers["Location"]