Merge pull request #1477 from basecamp/magic-links-cleanup

Magic links cleanup

app/controllers/concerns/authentication.rb

@@ -23,6 +23,7 @@ module Authentication
     def allow_unauthenticated_access(**options)
       skip_before_action :require_authentication, **options
       before_action :resume_session, **options
+      allow_unauthorized_access **options
     end
 
     def require_untenanted_access(**options)

app/controllers/concerns/authorization.rb

@@ -2,7 +2,7 @@ module Authorization
   extend ActiveSupport::Concern
 
   included do
-    before_action :ensure_can_access_account, if: -> { ApplicationRecord.current_tenant && Current.session }
+    before_action :ensure_can_access_account, if: -> { ApplicationRecord.current_tenant && authenticated? }
   end
 
   class_methods do

app/controllers/join_codes_controller.rb

@@ -2,6 +2,7 @@ class JoinCodesController < ApplicationController
   require_untenanted_access
   allow_unauthenticated_access
   before_action :set_join_code
+  before_action :ensure_join_code_is_valid
 
   def new
     @account_name = ApplicationRecord.with_tenant(tenant) { Account.sole.name }
@@ -14,10 +15,15 @@ class JoinCodesController < ApplicationController
       identity.send_magic_link
     end
 
+    session[:return_to_after_authenticating] = root_url(script_name: "/#{tenant}")
     redirect_to session_magic_link_path
   end
 
   private
+    def ensure_join_code_is_valid
+      head :not_found unless @join_code&.active?
+    end
+
     def set_join_code
       @join_code ||= ApplicationRecord.with_tenant(tenant) { Account::JoinCode.active.find_by(code: code) }
     end

app/controllers/notifications/unsubscribes_controller.rb

@@ -1,6 +1,5 @@
 class Notifications::UnsubscribesController < ApplicationController
   allow_unauthenticated_access
-  allow_unauthorized_access
   skip_before_action :verify_authenticity_token
 
   before_action :set_user

app/controllers/public/base_controller.rb

@@ -1,6 +1,5 @@
 class Public::BaseController < ApplicationController
   allow_unauthenticated_access
-  allow_unauthorized_access
 
   before_action :set_collection, :set_card, :set_public_cache_expiration
 

app/models/identity.rb

@@ -6,7 +6,6 @@ class Identity < UntenantedRecord
   has_many :sessions, dependent: :destroy
 
   normalizes :email_address, with: ->(value) { value.strip.downcase }
-  validates :email_address, presence: true
 
   def send_magic_link
     magic_links.create!.tap do |magic_link|

app/models/user.rb

@@ -3,8 +3,6 @@ class User < ApplicationRecord
     Mentionable, Named, Notifiable, Role, Searcher, Watcher
   include Timelined # Depends on Accessor
 
-  self.ignored_columns = %i[ password_digest ]
-
   has_one_attached :avatar
 
   belongs_to :membership, optional: true
@@ -18,8 +16,6 @@ class User < ApplicationRecord
   has_many :pins, dependent: :destroy
   has_many :pinned_cards, through: :pins, source: :card
 
-  normalizes :email_address, with: ->(value) { value.strip.downcase }
-
   delegate :staff?, to: :identity, allow_nil: true
 
   def deactivate

app/views/account/settings/_user.html.erb

@@ -3,7 +3,7 @@
     <%= avatar_preview_tag user, hidden_for_screen_reader: true %>
     <div class="txt-align-start overflow-ellipsis">
       <strong><%= user.name %></strong>
-      <div class="txt-x-small"><%= user.email_address %></div>
+      <div class="txt-x-small"><%= user.identity.email_address %></div>
     </div>
   <% end %>
 

app/views/collections/_access_toggle.erb

@@ -3,7 +3,7 @@
     <%= avatar_preview_tag user, hidden_for_screen_reader: true %>
     <div class="txt-align-start overflow-ellipsis">
       <strong><%= user.name %></strong>
-      <div class="txt-x-small"><%= user.email_address %></div>
+      <div class="txt-x-small"><%= user.identity.email_address %></div>
     </div>
   <% end %>
 

app/views/users/_user.json.jbuilder

@@ -1,6 +1,7 @@
 json.cache! user do
-  json.(user, :id, :name, :email_address, :role, :active)
+  json.(user, :id, :name, :role, :active)
 
+  json.email_address user.identity.email_address
   json.created_at user.created_at.utc
 
   json.url user_url(user)

app/views/users/show.html.erb

@@ -19,7 +19,7 @@
         <h1 class="txt-x-large margin-none"><%= @user.name %></h1>
         <div class="txt-medium">
           <% if @user.active? %>
-            <%= mail_to @user.email_address %>
+            <%= mail_to @user.identity.email_address %>
           <% else %>
             <%= @user.name %> is no longer on this account
           <% end %>

config/routes.rb

@@ -1,6 +1,5 @@
 Rails.application.routes.draw do
   namespace :account do
-    post :enter, to: "entries#create"
     resource :join_code
     resource :settings
     resource :entropy

db/migrate/20251103125353_remove_email_address_and_password_digest_from_users.rb

@@ -0,0 +1,6 @@
+class RemoveEmailAddressAndPasswordDigestFromUsers < ActiveRecord::Migration[8.2]
+  def change
+    remove_column :users, :email_address, :string
+    remove_column :users, :password_digest, :string
+  end
+end

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_02_115338) do
+ActiveRecord::Schema[8.2].define(version: 2025_11_03_125353) do
   create_table "accesses", force: :cascade do |t|
     t.datetime "accessed_at"
     t.integer "collection_id", null: false
@@ -389,13 +389,10 @@ ActiveRecord::Schema[8.2].define(version: 2025_11_02_115338) do
   create_table "users", force: :cascade do |t|
     t.boolean "active", default: true, null: false
     t.datetime "created_at", null: false
-    t.string "email_address"
     t.integer "membership_id"
     t.string "name", null: false
-    t.string "password_digest"
     t.string "role", default: "member", null: false
     t.datetime "updated_at", null: false
-    t.index ["email_address"], name: "index_users_on_email_address", unique: true
     t.index ["membership_id"], name: "index_users_on_membership_id"
     t.index ["role"], name: "index_users_on_role"
   end

db/schema_cache.yml

@@ -1045,16 +1045,6 @@ columns:
     collation:
     comment:
   - *7
-  - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column
-    auto_increment:
-    name: email_address
-    cast_type: *5
-    sql_type_metadata: *6
-    'null': true
-    default:
-    default_function:
-    collation:
-    comment:
   - *8
   - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column
     auto_increment:
@@ -1067,16 +1057,6 @@ columns:
     collation:
     comment:
   - *10
-  - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column
-    auto_increment:
-    name: password_digest
-    cast_type: *5
-    sql_type_metadata: *6
-    'null': true
-    default:
-    default_function:
-    collation:
-    comment:
   - !ruby/object:ActiveRecord::ConnectionAdapters::SQLite3::Column
     auto_increment:
     name: role
@@ -2715,22 +2695,6 @@ indexes:
     comment:
     valid: true
   users:
-  - !ruby/object:ActiveRecord::ConnectionAdapters::IndexDefinition
-    table: users
-    name: index_users_on_email_address
-    unique: true
-    columns:
-    - email_address
-    lengths: {}
-    orders: {}
-    opclasses: {}
-    where:
-    type:
-    using:
-    include:
-    nulls_not_distinct:
-    comment:
-    valid: true
   - !ruby/object:ActiveRecord::ConnectionAdapters::IndexDefinition
     table: users
     name: index_users_on_membership_id
@@ -2879,4 +2843,4 @@ indexes:
     nulls_not_distinct:
     comment:
     valid: true
-version: 20251102115338
+version: 20251103125353

db/seeds.rb

@@ -33,7 +33,7 @@ def create_tenant(signal_account_name)
 end
 
 def find_or_create_user(full_name, email_address)
-  if user = User.find_by(email_address: email_address)
+  if user = Identity.find_by(email_address: email_address)&.memberships&.find_by(tenant: ApplicationRecord.current_tenant)&.user
     user
   else
     identity = Identity.find_or_create_by!(email_address: email_address)

db/untenanted_migrate/20251103125952_make_email_addresses_mandatory_on_identities.rb

@@ -0,0 +1,5 @@
+class MakeEmailAddressesMandatoryOnIdentities < ActiveRecord::Migration[8.2]
+  def change
+    change_column_null :identities, :email_address, false
+  end
+end

db/untenanted_schema.rb

@@ -10,10 +10,10 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.2].define(version: 2025_10_27_131911) do
+ActiveRecord::Schema[8.2].define(version: 2025_11_03_125952) do
   create_table "identities", force: :cascade do |t|
     t.datetime "created_at", null: false
-    t.string "email_address"
+    t.string "email_address", null: false
     t.datetime "updated_at", null: false
     t.index ["email_address"], name: "index_identities_on_email_address", unique: true
   end

test/controllers/join_codes_controller_test.rb

@@ -0,0 +1,62 @@
+require "test_helper"
+
+class JoinCodesControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    @tenant = ApplicationRecord.current_tenant
+    @join_code = account_join_codes(:sole)
+  end
+
+  test "new" do
+    untenanted do
+      get join_path(tenant: @tenant, code: @join_code.code)
+    end
+
+    assert_response :success
+    assert_in_body "37signals"
+  end
+
+  test "new with an invalid code" do
+    untenanted do
+      get join_path(tenant: @tenant, code: "INVALID-CODE")
+    end
+
+    assert_response :not_found
+  end
+
+  test "new with an inactive code" do
+    @join_code.update!(usage_count: @join_code.usage_limit)
+
+    untenanted do
+      get join_path(tenant: @tenant, code: @join_code.code)
+    end
+
+    assert_response :not_found
+  end
+
+  test "create" do
+    untenanted do
+      assert_difference -> { Identity.count }, 1 do
+        assert_difference -> { Membership.count }, 1 do
+          post join_path(tenant: @tenant, code: @join_code.code), params: { email_address: "new_user@example.com" }
+        end
+      end
+
+      assert_redirected_to session_magic_link_path
+      assert_equal root_url(script_name: "/#{@tenant}"), session[:return_to_after_authenticating]
+    end
+  end
+
+  test "create for existing identity" do
+    identity = identities(:jz)
+
+    untenanted do
+      assert_no_difference -> { Identity.count } do
+        assert_difference -> { Membership.count }, 1 do
+          post join_path(tenant: @tenant, code: @join_code.code), params: { email_address: identity.email_address }
+        end
+      end
+
+      assert_redirected_to session_magic_link_path
+    end
+  end
+end

test/fixtures/account/join_codes.yml

@@ -1,5 +1,3 @@
-# Read about fixtures at https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html
-
 sole:
   code: 1234-5678-9XYZ
   usage_count: 0

test/models/account_test.rb

@@ -13,6 +13,8 @@ class AccountTest < ActiveSupport::TestCase
   end
 
   test ".create_with_admin_user creates a new local account" do
+    membership = memberships(:david_in_37signals)
+
     ApplicationRecord.create_tenant("account-create-with-dependents") do
       account = Account.create_with_admin_user(
         account: {
@@ -21,7 +23,7 @@ class AccountTest < ActiveSupport::TestCase
         },
         owner: {
           name: "David",
-          email_address: "david@37signals.com"
+          membership: membership
         }
       )
       assert_not_nil account
@@ -37,7 +39,7 @@ class AccountTest < ActiveSupport::TestCase
 
       admin = User.find_by(role: "admin")
       assert_equal "David", admin.name
-      assert_equal "david@37signals.com", admin.email_address
+      assert_equal "david@37signals.com", admin.identity.email_address
       assert_equal "admin", admin.role
     end
   end

test/models/user/accessor_test.rb

@@ -2,7 +2,7 @@ require "test_helper"
 
 class User::AccessorTest < ActiveSupport::TestCase
   test "new users get added to all_access collections on creation" do
-    user = User.create!(name: "Jorge", email_address: "testregular@example.com")
+    user = User.create!(name: "Jorge")
 
     assert_includes user.collections, collections(:writebook)
     assert_equal Collection.all_access.count, user.collections.count

test/models/user/configurable_test.rb

@@ -2,7 +2,7 @@ require "test_helper"
 
 class User::ConfigurableTest < ActiveSupport::TestCase
   test "should create settings for new users" do
-    user = User.create! name: "Some new user", email_address: "some.new@user.com"
+    user = User.create! name: "Some new user"
     assert user.settings.present?
   end
 end

test/models/user_test.rb

@@ -4,8 +4,7 @@ class UserTest < ActiveSupport::TestCase
   test "create" do
     user = User.create! \
       role: "member",
-      name: "Victor Cooper",
-      email_address: "victor@hey.com"
+      name: "Victor Cooper"
 
     assert_equal [ collections(:writebook) ], user.collections
     assert user.settings.present?
@@ -14,8 +13,7 @@ class UserTest < ActiveSupport::TestCase
   test "creation gives access to all_access collections" do
     user = User.create! \
       role: "member",
-      name: "Victor Cooper",
-      email_address: "victor@hey.com"
+      name: "Victor Cooper"
 
     assert_equal [ collections(:writebook) ], user.collections
   end