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
This commit is contained in:
@@ -68,7 +68,7 @@ Fizzy uses **URL path-based multi-tenancy**:
|
|||||||
|
|
||||||
**Passwordless magic link authentication**:
|
**Passwordless magic link authentication**:
|
||||||
- Global `Identity` (email-based) can have `Users` in multiple Accounts
|
- 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
|
- Sessions managed via signed cookies
|
||||||
- Board-level access control via `Access` records
|
- Board-level access control via `Access` records
|
||||||
|
|
||||||
@@ -84,7 +84,7 @@ Fizzy uses **URL path-based multi-tenancy**:
|
|||||||
|
|
||||||
**User** → Account membership
|
**User** → Account membership
|
||||||
- Belongs to Account and Identity
|
- Belongs to Account and Identity
|
||||||
- Has role (admin/member/system)
|
- Has role (owner/admin/member/system)
|
||||||
- Board access via explicit `Access` records
|
- Board access via explicit `Access` records
|
||||||
|
|
||||||
**Board** → Primary organizational unit
|
**Board** → Primary organizational unit
|
||||||
|
|||||||
@@ -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
|
||||||
@@ -17,10 +17,10 @@ class Account < ApplicationRecord
|
|||||||
validates :name, presence: true
|
validates :name, presence: true
|
||||||
|
|
||||||
class << self
|
class << self
|
||||||
def create_with_admin_user(account:, owner:)
|
def create_with_owner(account:, owner:)
|
||||||
create!(**account).tap do |account|
|
create!(**account).tap do |account|
|
||||||
account.users.create!(role: :system, name: "System")
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -2,6 +2,6 @@ module Account::Seedeable
|
|||||||
extend ActiveSupport::Concern
|
extend ActiveSupport::Concern
|
||||||
|
|
||||||
def setup_customer_template
|
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
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -54,7 +54,7 @@ class Signup
|
|||||||
end
|
end
|
||||||
|
|
||||||
def create_account
|
def create_account
|
||||||
@account = Account.create_with_admin_user(
|
@account = Account.create_with_owner(
|
||||||
account: {
|
account: {
|
||||||
external_account_id: @tenant,
|
external_account_id: @tenant,
|
||||||
name: generate_account_name
|
name: generate_account_name
|
||||||
@@ -64,7 +64,7 @@ class Signup
|
|||||||
identity: identity
|
identity: identity
|
||||||
}
|
}
|
||||||
)
|
)
|
||||||
@user = @account.users.find_by!(role: :admin)
|
@user = @account.users.find_by!(role: :owner)
|
||||||
@account.setup_customer_template
|
@account.setup_customer_template
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|||||||
+11
-5
@@ -2,18 +2,24 @@ module User::Role
|
|||||||
extend ActiveSupport::Concern
|
extend ActiveSupport::Concern
|
||||||
|
|
||||||
included do
|
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 :owner, -> { where(active: true, role: :owner) }
|
||||||
scope :active, -> { where(active: true, role: %i[ admin member ]) }
|
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
|
end
|
||||||
|
|
||||||
def can_change?(other)
|
def can_change?(other)
|
||||||
admin? || other == self
|
(admin? && !other.owner?) || other == self
|
||||||
end
|
end
|
||||||
|
|
||||||
def can_administer?(other)
|
def can_administer?(other)
|
||||||
admin? && other != self
|
admin? && !other.owner? && other != self
|
||||||
end
|
end
|
||||||
|
|
||||||
def can_administer_board?(board)
|
def can_administer_board?(board)
|
||||||
|
|||||||
@@ -10,11 +10,13 @@
|
|||||||
<hr class="separator--horizontal flex-item-grow" style="--border-color: var(--color-ink-medium); --border-style: dashed" aria-hidden="true">
|
<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 | %>
|
<%= 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" %>">
|
<span title="<%= role_display_name(user) %>">
|
||||||
<%= icon_tag "crown" %>
|
<label class="btn btn--circle" for="<%= dom_id(user, :role) %>" aria-label="Role: <%= role_display_name(user) %>">
|
||||||
<span class="for-screen-reader">Role: <%= user.admin? ? "Administrator" : "Member" %></span>
|
<%= icon_tag "crown" %>
|
||||||
<%= form.check_box :role, { data: { action: "form#submit" }, disabled: !Current.user.can_administer?(user), hidden: true, id: dom_id(user, :role) }, "admin", "member" %>
|
<span class="for-screen-reader">Role: <%= role_display_name(user) %></span>
|
||||||
</label>
|
<%= 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 %>
|
<% 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 %>
|
<%# 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 %>
|
||||||
|
|||||||
@@ -70,7 +70,7 @@
|
|||||||
|
|
||||||
<ul class="margin-block-half">
|
<ul class="margin-block-half">
|
||||||
<% @top_accounts.each do |account| %>
|
<% @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">
|
<li class="flex align-start gap-half margin-block-start-half">
|
||||||
<div
|
<div
|
||||||
class="flex-item-grow min-width overflow-ellipsis"
|
class="flex-item-grow min-width overflow-ellipsis"
|
||||||
|
|||||||
@@ -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
|
||||||
Generated
+1
-1
@@ -10,7 +10,7 @@
|
|||||||
#
|
#
|
||||||
# It's strongly recommended that you check this file into your version control system.
|
# 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|
|
create_table "accesses", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
|
||||||
t.datetime "accessed_at"
|
t.datetime "accessed_at"
|
||||||
t.uuid "account_id", null: false
|
t.uuid "account_id", null: false
|
||||||
|
|||||||
+1
-1
@@ -10,7 +10,7 @@
|
|||||||
#
|
#
|
||||||
# It's strongly recommended that you check this file into your version control system.
|
# 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|
|
create_table "accesses", id: :uuid, force: :cascade do |t|
|
||||||
t.datetime "accessed_at"
|
t.datetime "accessed_at"
|
||||||
t.uuid "account_id", null: false
|
t.uuid "account_id", null: false
|
||||||
|
|||||||
+1
-1
@@ -17,7 +17,7 @@ else
|
|||||||
identity = Identity.find_or_create_by!(email_address: email_address)
|
identity = Identity.find_or_create_by!(email_address: email_address)
|
||||||
|
|
||||||
unless account = Account.find_by(external_account_id: tenant_id)
|
unless account = Account.find_by(external_account_id: tenant_id)
|
||||||
account = Account.create_with_admin_user(
|
account = Account.create_with_owner(
|
||||||
account: {
|
account: {
|
||||||
external_account_id: tenant_id,
|
external_account_id: tenant_id,
|
||||||
name: signal_account_name
|
name: signal_account_name
|
||||||
|
|||||||
@@ -122,7 +122,7 @@ class Import
|
|||||||
|
|
||||||
def setup_account
|
def setup_account
|
||||||
step("Setting up account", "Account set up in %{duration}") do
|
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
|
raise "No admin user found in the database" unless oldest_admin
|
||||||
|
|
||||||
membership = untenanted.memberships.find(oldest_admin.membership_id)
|
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)
|
if Account.all.exists?(external_account_id: account.external_account_id)
|
||||||
raise AccountExistsError, "Account already exists"
|
raise AccountExistsError, "Account already exists"
|
||||||
else
|
else
|
||||||
@account = Account.create_with_admin_user(
|
@account = Account.create_with_owner(
|
||||||
account: {
|
account: {
|
||||||
external_account_id: account.external_account_id,
|
external_account_id: account.external_account_id,
|
||||||
name: account.name.truncate(255, omission: "")
|
name: account.name.truncate(255, omission: "")
|
||||||
@@ -144,7 +144,7 @@ class Import
|
|||||||
}
|
}
|
||||||
)
|
)
|
||||||
@tenant = @account.external_account_id
|
@tenant = @account.external_account_id
|
||||||
@admin = @account.users.find_by(role: :admin)
|
@admin = @account.users.find_by(role: :owner)
|
||||||
end
|
end
|
||||||
|
|
||||||
old_join_code = import.account_join_codes.sole
|
old_join_code = import.account_join_codes.sole
|
||||||
|
|||||||
@@ -37,11 +37,11 @@ tenant = ActiveRecord::FixtureSet.identify(identifier)
|
|||||||
ApplicationRecord.with_tenant(tenant) do |tenant|
|
ApplicationRecord.with_tenant(tenant) do |tenant|
|
||||||
Current.account.destroy!
|
Current.account.destroy!
|
||||||
|
|
||||||
Account.create_with_admin_user \
|
Account.create_with_owner \
|
||||||
account: { name: "Company #{identifier}" },
|
account: { name: "Company #{identifier}" },
|
||||||
owner: { name: "Developer #{identifier}", email_address: "dev-#{identifier}@example.com" }
|
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 = Identity.find_or_create_by(email_address: user.email_address)
|
||||||
identity.link_to(user.tenant)
|
identity.link_to(user.tenant)
|
||||||
Board.find_each do |board|
|
Board.find_each do |board|
|
||||||
|
|||||||
@@ -18,5 +18,29 @@ class Users::RolesControllerTest < ActionDispatch::IntegrationTest
|
|||||||
assert_no_changes -> { users(:david).reload.role } do
|
assert_no_changes -> { users(:david).reload.role } do
|
||||||
put user_role_path(users(:david)), params: { user: { role: "system" } }
|
put user_role_path(users(:david)), params: { user: { role: "system" } }
|
||||||
end
|
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
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -41,6 +41,20 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
|
|||||||
assert_nil User.active.find_by(id: users(:david).id)
|
assert_nil User.active.find_by(id: users(:david).id)
|
||||||
end
|
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
|
test "non-admins cannot perform actions" do
|
||||||
sign_in_as :jz
|
sign_in_as :jz
|
||||||
|
|
||||||
|
|||||||
Vendored
+4
@@ -5,6 +5,10 @@ david:
|
|||||||
jz:
|
jz:
|
||||||
email_address: jz@37signals.com
|
email_address: jz@37signals.com
|
||||||
|
|
||||||
|
jason:
|
||||||
|
email_address: jason@37signals.com
|
||||||
|
staff: true
|
||||||
|
|
||||||
kevin:
|
kevin:
|
||||||
email_address: kevin@37signals.com
|
email_address: kevin@37signals.com
|
||||||
staff: true
|
staff: true
|
||||||
|
|||||||
Vendored
+7
@@ -12,6 +12,13 @@ jz:
|
|||||||
identity: jz
|
identity: jz
|
||||||
account: 37s_uuid
|
account: 37s_uuid
|
||||||
|
|
||||||
|
jason:
|
||||||
|
id: <%= ActiveRecord::FixtureSet.identify("jason", :uuid) %>
|
||||||
|
name: Jason
|
||||||
|
role: owner
|
||||||
|
identity: jason
|
||||||
|
account: 37s_uuid
|
||||||
|
|
||||||
kevin:
|
kevin:
|
||||||
id: <%= ActiveRecord::FixtureSet.identify("kevin", :uuid) %>
|
id: <%= ActiveRecord::FixtureSet.identify("kevin", :uuid) %>
|
||||||
name: Kevin
|
name: Kevin
|
||||||
|
|||||||
+11
-10
@@ -12,17 +12,17 @@ class AccountTest < ActiveSupport::TestCase
|
|||||||
assert_equal "/#{account.external_account_id}", account.slug
|
assert_equal "/#{account.external_account_id}", account.slug
|
||||||
end
|
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
|
Current.without_account do
|
||||||
identity = identities(:david)
|
identity = identities(:david)
|
||||||
account = nil
|
account = nil
|
||||||
|
|
||||||
assert_changes -> { Account.count }, +1 do
|
assert_changes -> { Account.count }, +1 do
|
||||||
assert_changes -> { User.count }, +2 do
|
assert_changes -> { User.count }, +2 do
|
||||||
account = Account.create_with_admin_user(
|
account = Account.create_with_owner(
|
||||||
account: {
|
account: {
|
||||||
external_account_id: ActiveRecord::FixtureSet.identify("account-create-with-admin-user-test"),
|
external_account_id: ActiveRecord::FixtureSet.identify("account-create-with-owner-test"),
|
||||||
name: "Account Create With Admin"
|
name: "Account Create With Owner"
|
||||||
},
|
},
|
||||||
owner: {
|
owner: {
|
||||||
name: "David",
|
name: "David",
|
||||||
@@ -34,13 +34,14 @@ class AccountTest < ActiveSupport::TestCase
|
|||||||
|
|
||||||
assert_not_nil account
|
assert_not_nil account
|
||||||
assert account.persisted?
|
assert account.persisted?
|
||||||
assert_equal ActiveRecord::FixtureSet.identify("account-create-with-admin-user-test"), account.external_account_id
|
assert_equal ActiveRecord::FixtureSet.identify("account-create-with-owner-test"), account.external_account_id
|
||||||
assert_equal "Account Create With Admin", account.name
|
assert_equal "Account Create With Owner", account.name
|
||||||
|
|
||||||
admin = account.users.find_by(role: "admin")
|
owner = account.users.find_by(role: "owner")
|
||||||
assert_equal "David", admin.name
|
assert_equal "David", owner.name
|
||||||
assert_equal "david@37signals.com", admin.identity.email_address
|
assert_equal "david@37signals.com", owner.identity.email_address
|
||||||
assert_equal "admin", admin.role
|
assert_equal "owner", owner.role
|
||||||
|
assert owner.admin?, "owner should also be considered an admin"
|
||||||
|
|
||||||
assert_predicate account.system_user, :present?
|
assert_predicate account.system_user, :present?
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -8,6 +8,56 @@ class User::RoleTest < ActiveSupport::TestCase
|
|||||||
assert_not users(:jz).can_administer?(users(:kevin))
|
assert_not users(:jz).can_administer?(users(:kevin))
|
||||||
end
|
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
|
test "can administer board?" do
|
||||||
writebook_board = boards(:writebook)
|
writebook_board = boards(:writebook)
|
||||||
private_board = boards(:private)
|
private_board = boards(:private)
|
||||||
|
|||||||
Reference in New Issue
Block a user