From edf6b53469270f38b59d85ee0f8a03239a7c3e19 Mon Sep 17 00:00:00 2001
From: Mike Dalessio <mike@37signals.com>
Date: Sat, 29 Nov 2025 13:17:59 -0500
Subject: [PATCH] Introduce an "owner" role, and prevent it from being
 administered

to prevent the owner from being demoted or kicked out.

ref: https://app.fizzy.do/5986089/cards/3213
---
 AGENTS.md                                     |  4 +-
 app/helpers/users_helper.rb                   |  8 +++
 app/models/account.rb                         |  4 +-
 app/models/account/seedeable.rb               |  2 +-
 app/models/signup.rb                          |  4 +-
 app/models/user/role.rb                       | 16 ++++--
 app/views/account/settings/_user.html.erb     | 12 +++--
 app/views/admin/stats/show.html.erb           |  2 +-
 ...1129175717_promote_first_admin_to_owner.rb | 14 ++++++
 db/schema.rb                                  |  2 +-
 db/schema_sqlite.rb                           |  2 +-
 db/seeds.rb                                   |  2 +-
 script/import-sqlite-database.rb              |  6 +--
 script/load-prod-db-in-dev.rb                 |  4 +-
 .../users/roles_controller_test.rb            | 24 +++++++++
 test/controllers/users_controller_test.rb     | 14 ++++++
 test/fixtures/identities.yml                  |  4 ++
 test/fixtures/users.yml                       |  7 +++
 test/models/account_test.rb                   | 21 ++++----
 test/models/user/role_test.rb                 | 50 +++++++++++++++++++
 20 files changed, 166 insertions(+), 36 deletions(-)
 create mode 100644 app/helpers/users_helper.rb
 create mode 100644 db/migrate/20251129175717_promote_first_admin_to_owner.rb

diff --git a/AGENTS.md b/AGENTS.md
index 8bdaa4481..203422200 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -68,7 +68,7 @@ Fizzy uses **URL path-based multi-tenancy**:
 
 **Passwordless magic link authentication**:
 - Global `Identity` (email-based) can have `Users` in multiple Accounts
-- Users belong to an Account and have roles: admin, member, system
+- Users belong to an Account and have roles: owner, admin, member, system
 - Sessions managed via signed cookies
 - Board-level access control via `Access` records
 
@@ -84,7 +84,7 @@ Fizzy uses **URL path-based multi-tenancy**:
 
 **User** → Account membership
 - Belongs to Account and Identity
-- Has role (admin/member/system)
+- Has role (owner/admin/member/system)
 - Board access via explicit `Access` records
 
 **Board** → Primary organizational unit
diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb
new file mode 100644
index 000000000..7cccfe4ec
--- /dev/null
+++ b/app/helpers/users_helper.rb
@@ -0,0 +1,8 @@
+module UsersHelper
+  def role_display_name(user)
+    case user.role
+    when "admin" then "Administrator"
+    else user.role.titleize
+    end
+  end
+end
diff --git a/app/models/account.rb b/app/models/account.rb
index 887ce708d..9c445f10e 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -17,10 +17,10 @@ class Account < ApplicationRecord
   validates :name, presence: true
 
   class << self
-    def create_with_admin_user(account:, owner:)
+    def create_with_owner(account:, owner:)
       create!(**account).tap do |account|
         account.users.create!(role: :system, name: "System")
-        account.users.create!(**owner.reverse_merge(role: "admin"))
+        account.users.create!(**owner.reverse_merge(role: "owner"))
       end
     end
   end
diff --git a/app/models/account/seedeable.rb b/app/models/account/seedeable.rb
index f0592020a..e50c22e96 100644
--- a/app/models/account/seedeable.rb
+++ b/app/models/account/seedeable.rb
@@ -2,6 +2,6 @@ module Account::Seedeable
   extend ActiveSupport::Concern
 
   def setup_customer_template
-    Account::Seeder.new(self, users.active.where(role: :admin).first).seed
+    Account::Seeder.new(self, users.admin.first).seed
   end
 end
diff --git a/app/models/signup.rb b/app/models/signup.rb
index bdcf3490d..15ac5873c 100644
--- a/app/models/signup.rb
+++ b/app/models/signup.rb
@@ -54,7 +54,7 @@ class Signup
     end
 
     def create_account
-      @account = Account.create_with_admin_user(
+      @account = Account.create_with_owner(
         account: {
           external_account_id: @tenant,
           name: generate_account_name
@@ -64,7 +64,7 @@ class Signup
           identity: identity
         }
       )
-      @user = @account.users.find_by!(role: :admin)
+      @user = @account.users.find_by!(role: :owner)
       @account.setup_customer_template
     end
 
diff --git a/app/models/user/role.rb b/app/models/user/role.rb
index 3c2101f68..e103e94d9 100644
--- a/app/models/user/role.rb
+++ b/app/models/user/role.rb
@@ -2,18 +2,24 @@ module User::Role
   extend ActiveSupport::Concern
 
   included do
-    enum :role, %i[ admin member system ].index_by(&:itself), scopes: false
+    enum :role, %i[ owner admin member system ].index_by(&:itself), scopes: false
 
-    scope :member, -> { where(role: :member) }
-    scope :active, -> { where(active: true, role: %i[ admin member ]) }
+    scope :owner, -> { where(active: true, role: :owner) }
+    scope :admin, -> { where(active: true, role: %i[ owner admin ]) }
+    scope :member, -> { where(active: true, role: :member) }
+    scope :active, -> { where(active: true, role: %i[ owner admin member ]) }
+
+    def admin?
+      super || owner?
+    end
   end
 
   def can_change?(other)
-    admin? || other == self
+    (admin? && !other.owner?) || other == self
   end
 
   def can_administer?(other)
-    admin? && other != self
+    admin? && !other.owner? && other != self
   end
 
   def can_administer_board?(board)
diff --git a/app/views/account/settings/_user.html.erb b/app/views/account/settings/_user.html.erb
index 45fc77c07..6bc65acd8 100644
--- a/app/views/account/settings/_user.html.erb
+++ b/app/views/account/settings/_user.html.erb
@@ -10,11 +10,13 @@
   <hr class="separator--horizontal flex-item-grow" style="--border-color: var(--color-ink-medium); --border-style: dashed" aria-hidden="true">
 
   <%= form_with model: user, url: user_role_path(user), data: { controller: "form" }, method: :patch do | form | %>
-    <label class="btn btn--circle" for="<%= dom_id(user, :role) %>" arial-label="Role: <%= user.admin? ? "Administrator" : "Member" %>">
-      <%= icon_tag "crown" %>
-      <span class="for-screen-reader">Role: <%= user.admin? ? "Administrator" : "Member" %></span>
-      <%= form.check_box :role, { data: { action: "form#submit" }, disabled: !Current.user.can_administer?(user), hidden: true, id: dom_id(user, :role) }, "admin", "member" %>
-    </label>
+    <span title="<%= role_display_name(user) %>">
+      <label class="btn btn--circle" for="<%= dom_id(user, :role) %>" aria-label="Role: <%= role_display_name(user) %>">
+        <%= icon_tag "crown" %>
+        <span class="for-screen-reader">Role: <%= role_display_name(user) %></span>
+        <%= form.check_box :role, { data: { action: "form#submit" }, disabled: !Current.user.can_administer?(user), checked: user.admin?, hidden: true, id: dom_id(user, :role) }, "admin", "member" %>
+      </label>
+    </span>
   <% end %>
 
   <%# FIXME: Move this Current.user check to a stimulus controller that just checks for admin? or the like we so we can cache user list %>
diff --git a/app/views/admin/stats/show.html.erb b/app/views/admin/stats/show.html.erb
index 2fe7ce71a..a3ae18c55 100644
--- a/app/views/admin/stats/show.html.erb
+++ b/app/views/admin/stats/show.html.erb
@@ -70,7 +70,7 @@
 
     <ul class="margin-block-half">
       <% @top_accounts.each do |account| %>
-        <% admin_user = account.users.find { |u| u.role == "admin" } %>
+        <% admin_user = account.users.owner.first %>
         <li class="flex align-start gap-half margin-block-start-half">
           <div
             class="flex-item-grow min-width overflow-ellipsis"
diff --git a/db/migrate/20251129175717_promote_first_admin_to_owner.rb b/db/migrate/20251129175717_promote_first_admin_to_owner.rb
new file mode 100644
index 000000000..8046367e7
--- /dev/null
+++ b/db/migrate/20251129175717_promote_first_admin_to_owner.rb
@@ -0,0 +1,14 @@
+class PromoteFirstAdminToOwner < ActiveRecord::Migration[8.2]
+  def up
+    Account.find_each do |account|
+      next if account.users.exists?(role: :owner)
+
+      first_admin = account.users.where(role: :admin).order(:created_at).first
+      first_admin&.update!(role: :owner)
+    end
+  end
+
+  def down
+    User.where(role: :owner).update_all(role: :admin)
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index ef0a4d166..52a460e71 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.2].define(version: 2025_11_29_110120) do
+ActiveRecord::Schema[8.2].define(version: 2025_11_29_175717) do
   create_table "accesses", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
     t.datetime "accessed_at"
     t.uuid "account_id", null: false
diff --git a/db/schema_sqlite.rb b/db/schema_sqlite.rb
index 149862250..1a4a85e1d 100644
--- a/db/schema_sqlite.rb
+++ b/db/schema_sqlite.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.2].define(version: 2025_11_29_110120) do
+ActiveRecord::Schema[8.2].define(version: 2025_11_29_175717) do
   create_table "accesses", id: :uuid, force: :cascade do |t|
     t.datetime "accessed_at"
     t.uuid "account_id", null: false
diff --git a/db/seeds.rb b/db/seeds.rb
index 80523e86e..422921309 100644
--- a/db/seeds.rb
+++ b/db/seeds.rb
@@ -17,7 +17,7 @@ else
     identity = Identity.find_or_create_by!(email_address: email_address)
 
     unless account = Account.find_by(external_account_id: tenant_id)
-      account = Account.create_with_admin_user(
+      account = Account.create_with_owner(
         account: {
           external_account_id: tenant_id,
           name: signal_account_name
diff --git a/script/import-sqlite-database.rb b/script/import-sqlite-database.rb
index ac5a5f200..d94175bb0 100755
--- a/script/import-sqlite-database.rb
+++ b/script/import-sqlite-database.rb
@@ -122,7 +122,7 @@ class Import
 
     def setup_account
       step("Setting up account", "Account set up in %{duration}") do
-        oldest_admin = import.users.order(id: :asc).where(role: :admin, active: true).first
+        oldest_admin = import.users.order(id: :asc).admin.first
         raise "No admin user found in the database" unless oldest_admin
 
         membership = untenanted.memberships.find(oldest_admin.membership_id)
@@ -133,7 +133,7 @@ class Import
         if Account.all.exists?(external_account_id: account.external_account_id)
           raise AccountExistsError, "Account already exists"
         else
-          @account = Account.create_with_admin_user(
+          @account = Account.create_with_owner(
             account: {
               external_account_id: account.external_account_id,
               name: account.name.truncate(255, omission: "")
@@ -144,7 +144,7 @@ class Import
             }
           )
           @tenant = @account.external_account_id
-          @admin = @account.users.find_by(role: :admin)
+          @admin = @account.users.find_by(role: :owner)
         end
 
         old_join_code = import.account_join_codes.sole
diff --git a/script/load-prod-db-in-dev.rb b/script/load-prod-db-in-dev.rb
index 0bf17c6e7..c01063d20 100755
--- a/script/load-prod-db-in-dev.rb
+++ b/script/load-prod-db-in-dev.rb
@@ -37,11 +37,11 @@ tenant = ActiveRecord::FixtureSet.identify(identifier)
 ApplicationRecord.with_tenant(tenant) do |tenant|
   Current.account.destroy!
 
-  Account.create_with_admin_user \
+  Account.create_with_owner \
     account: { name: "Company #{identifier}" },
     owner: { name: "Developer #{identifier}", email_address: "dev-#{identifier}@example.com" }
 
-  user = User.find_by(role: :admin)
+  user = User.find_by(role: :owner)
   identity = Identity.find_or_create_by(email_address: user.email_address)
   identity.link_to(user.tenant)
   Board.find_each do |board|
diff --git a/test/controllers/users/roles_controller_test.rb b/test/controllers/users/roles_controller_test.rb
index ceaf6e2fb..63962c416 100644
--- a/test/controllers/users/roles_controller_test.rb
+++ b/test/controllers/users/roles_controller_test.rb
@@ -18,5 +18,29 @@ class Users::RolesControllerTest < ActionDispatch::IntegrationTest
     assert_no_changes -> { users(:david).reload.role } do
       put user_role_path(users(:david)), params: { user: { role: "system" } }
     end
+
+    assert_no_changes -> { users(:david).reload.role } do
+      put user_role_path(users(:david)), params: { user: { role: "owner" } }
+    end
+  end
+
+  test "admin cannot demote the owner" do
+    assert users(:jason).owner?
+
+    assert_no_changes -> { users(:jason).reload.role } do
+      put user_role_path(users(:jason)), params: { user: { role: "admin" } }
+    end
+
+    assert_response :forbidden
+  end
+
+  test "admin cannot change owner role to member" do
+    assert users(:jason).owner?
+
+    assert_no_changes -> { users(:jason).reload.role } do
+      put user_role_path(users(:jason)), params: { user: { role: "member" } }
+    end
+
+    assert_response :forbidden
   end
 end
diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb
index 2c835984c..081579dfc 100644
--- a/test/controllers/users_controller_test.rb
+++ b/test/controllers/users_controller_test.rb
@@ -41,6 +41,20 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
     assert_nil User.active.find_by(id: users(:david).id)
   end
 
+  test "admin cannot deactivate the owner" do
+    sign_in_as :kevin
+
+    assert users(:jason).owner?
+    assert users(:jason).active
+
+    assert_no_difference -> { User.active.count } do
+      delete user_path(users(:jason))
+    end
+
+    assert_response :forbidden
+    assert users(:jason).reload.active
+  end
+
   test "non-admins cannot perform actions" do
     sign_in_as :jz
 
diff --git a/test/fixtures/identities.yml b/test/fixtures/identities.yml
index af4c2a060..bf5260c03 100644
--- a/test/fixtures/identities.yml
+++ b/test/fixtures/identities.yml
@@ -5,6 +5,10 @@ david:
 jz:
   email_address: jz@37signals.com
 
+jason:
+  email_address: jason@37signals.com
+  staff: true
+
 kevin:
   email_address: kevin@37signals.com
   staff: true
diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml
index 70a855634..4c7dad414 100644
--- a/test/fixtures/users.yml
+++ b/test/fixtures/users.yml
@@ -12,6 +12,13 @@ jz:
   identity: jz
   account: 37s_uuid
 
+jason:
+  id: <%= ActiveRecord::FixtureSet.identify("jason", :uuid) %>
+  name: Jason
+  role: owner
+  identity: jason
+  account: 37s_uuid
+
 kevin:
   id: <%= ActiveRecord::FixtureSet.identify("kevin", :uuid) %>
   name: Kevin
diff --git a/test/models/account_test.rb b/test/models/account_test.rb
index 541ceb654..490d6e32e 100644
--- a/test/models/account_test.rb
+++ b/test/models/account_test.rb
@@ -12,17 +12,17 @@ class AccountTest < ActiveSupport::TestCase
     assert_equal "/#{account.external_account_id}", account.slug
   end
 
-  test ".create_with_admin_user creates a new local account" do
+  test ".create_with_owner creates a new local account" do
     Current.without_account do
       identity = identities(:david)
       account = nil
 
       assert_changes -> { Account.count }, +1 do
         assert_changes -> { User.count }, +2 do
-          account = Account.create_with_admin_user(
+          account = Account.create_with_owner(
             account: {
-              external_account_id: ActiveRecord::FixtureSet.identify("account-create-with-admin-user-test"),
-              name: "Account Create With Admin"
+              external_account_id: ActiveRecord::FixtureSet.identify("account-create-with-owner-test"),
+              name: "Account Create With Owner"
             },
             owner: {
               name: "David",
@@ -34,13 +34,14 @@ class AccountTest < ActiveSupport::TestCase
 
       assert_not_nil account
       assert account.persisted?
-      assert_equal ActiveRecord::FixtureSet.identify("account-create-with-admin-user-test"), account.external_account_id
-      assert_equal "Account Create With Admin", account.name
+      assert_equal ActiveRecord::FixtureSet.identify("account-create-with-owner-test"), account.external_account_id
+      assert_equal "Account Create With Owner", account.name
 
-      admin = account.users.find_by(role: "admin")
-      assert_equal "David", admin.name
-      assert_equal "david@37signals.com", admin.identity.email_address
-      assert_equal "admin", admin.role
+      owner = account.users.find_by(role: "owner")
+      assert_equal "David", owner.name
+      assert_equal "david@37signals.com", owner.identity.email_address
+      assert_equal "owner", owner.role
+      assert owner.admin?, "owner should also be considered an admin"
 
       assert_predicate account.system_user, :present?
     end
diff --git a/test/models/user/role_test.rb b/test/models/user/role_test.rb
index 9f0889f9e..b76ce40c7 100644
--- a/test/models/user/role_test.rb
+++ b/test/models/user/role_test.rb
@@ -8,6 +8,56 @@ class User::RoleTest < ActiveSupport::TestCase
     assert_not users(:jz).can_administer?(users(:kevin))
   end
 
+  test "owner can administer admins and members" do
+    assert users(:jason).can_administer?(users(:kevin))
+    assert users(:jason).can_administer?(users(:david))
+    assert users(:jason).can_administer?(users(:jz))
+  end
+
+  test "owner cannot administer themselves" do
+    assert_not users(:jason).can_administer?(users(:jason))
+  end
+
+  test "admin cannot administer the owner" do
+    assert_not users(:kevin).can_administer?(users(:jason))
+  end
+
+  test "owner is included in active scope" do
+    active_users = User.active
+    assert_includes active_users, users(:jason)
+    assert_includes active_users, users(:kevin)
+    assert_includes active_users, users(:david)
+    assert_not_includes active_users, users(:system)
+  end
+
+  test "owner is also considered an admin" do
+    assert users(:jason).owner?
+    assert users(:jason).admin?
+
+    assert users(:kevin).admin?
+    assert_not users(:kevin).owner?
+  end
+
+  test "owner scope returns only active owners" do
+    owners = accounts("37s").users.owner
+    assert_includes owners, users(:jason)
+    assert_not_includes owners, users(:kevin)
+    assert_not_includes owners, users(:david)
+
+    users(:jason).update!(active: false)
+    assert_not_includes accounts("37s").users.owner, users(:jason)
+  end
+
+  test "admin scope returns active owners and admins" do
+    admins = accounts("37s").users.admin
+    assert_includes admins, users(:jason)
+    assert_includes admins, users(:kevin)
+    assert_not_includes admins, users(:david)
+
+    users(:kevin).update!(active: false)
+    assert_not_includes accounts("37s").users.admin, users(:kevin)
+  end
+
   test "can administer board?" do
     writebook_board = boards(:writebook)
     private_board = boards(:private)