diff --git a/app/controllers/api/v1/auth_controller.rb b/app/controllers/api/v1/auth_controller.rb index 4ced6ae52..49515a58b 100644 --- a/app/controllers/api/v1/auth_controller.rb +++ b/app/controllers/api/v1/auth_controller.rb @@ -44,7 +44,6 @@ module Api # First user of an instance becomes super_admin family = Family.new user.family = family - user.role = User.role_for_new_family_creator # Atomic: user creation, invite-code claim, and device/token issuance # either all commit or none do. Without this, a post-commit device @@ -53,6 +52,9 @@ module Api token_response = nil begin ActiveRecord::Base.transaction do + User.lock_first_user_role! + user.role = User.role_for_new_family_creator + unless user.save render json: { errors: user.errors.full_messages }, status: :unprocessable_entity raise ActiveRecord::Rollback @@ -215,7 +217,6 @@ module Api # while intentional super_admin defaults remain supported. provider_config = Rails.configuration.x.auth.sso_providers&.find { |p| p[:name] == cached[:provider] } provider_default_role = provider_config&.dig(:settings, :default_role) - user.role = User.role_for_new_family_creator(fallback_role: provider_default_role || :admin) end identity = nil @@ -223,6 +224,11 @@ module Api begin account_created = ActiveRecord::Base.transaction do + unless invitation.present? + User.lock_first_user_role! + user.role = User.role_for_new_family_creator(fallback_role: provider_default_role || :admin) + end + unless user.save raise ActiveRecord::Rollback end diff --git a/app/controllers/oidc_accounts_controller.rb b/app/controllers/oidc_accounts_controller.rb index e218edfeb..24e03fccd 100644 --- a/app/controllers/oidc_accounts_controller.rb +++ b/app/controllers/oidc_accounts_controller.rb @@ -147,7 +147,6 @@ class OidcAccountsController < ApplicationController # while intentional super_admin defaults remain supported. provider_config = Rails.configuration.x.auth.sso_providers&.find { |p| p[:name] == @pending_auth["provider"] } provider_default_role = provider_config&.dig(:settings, :default_role) - @user.role = User.role_for_new_family_creator(fallback_role: provider_default_role || :admin) end identity = nil @@ -155,6 +154,11 @@ class OidcAccountsController < ApplicationController begin account_created = ActiveRecord::Base.transaction do + unless invitation.present? + User.lock_first_user_role! + @user.role = User.role_for_new_family_creator(fallback_role: provider_default_role || :admin) + end + unless @user.save raise ActiveRecord::Rollback end diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 8cdff932d..66f14403b 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -25,7 +25,7 @@ class RegistrationsController < ApplicationController else family = Family.new @user.family = family - @user.role = User.role_for_new_family_creator + @creating_new_family = true end if signup_with_invite_claim! @@ -63,6 +63,11 @@ class RegistrationsController < ApplicationController success = false ActiveRecord::Base.transaction do + if @creating_new_family + User.lock_first_user_role! + @user.role = User.role_for_new_family_creator + end + unless @user.save raise ActiveRecord::Rollback end diff --git a/app/models/user.rb b/app/models/user.rb index 223cfde2f..67549ff6d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -78,6 +78,14 @@ class User < ApplicationRecord # Returns the appropriate role for a new user creating a family. # The very first user of an instance becomes super_admin; subsequent users # get the specified admin-capable fallback role. + # Keep this one-key advisory lock stable across deploys so old and new app + # processes serialize first-user role selection on the same database lock. + FIRST_USER_ROLE_LOCK_KEY = 8_391_247 + + def self.lock_first_user_role! + connection.execute(sanitize_sql_array([ "SELECT pg_advisory_xact_lock(?)", FIRST_USER_ROLE_LOCK_KEY ])) + end + def self.role_for_new_family_creator(fallback_role: :admin) fallback_role = fallback_role.to_s.in?(%w[admin super_admin]) ? fallback_role : :admin diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 913c65ed0..dfcb33cc5 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -755,14 +755,14 @@ components: type: string display_budgeted_spending_cents: type: integer - rolled_over_amount: - type: string - rolled_over_amount_cents: - type: integer actual_spending: type: string actual_spending_cents: type: integer + rolled_over_amount: + type: string + rolled_over_amount_cents: + type: integer available_to_spend: type: string available_to_spend_cents: @@ -3381,6 +3381,9 @@ paths: summary: Sign up a new user tags: - Auth + description: Creates a new user and family. The first user on a fresh instance + is assigned the super_admin role; later family creators are assigned an admin + role. parameters: [] responses: '201': @@ -3788,7 +3791,9 @@ paths: description: Creates a new user and family from a previously issued linking code. Links the SSO identity via OidcIdentity, logs the JIT account creation via SsoAuditLog, and issues mobile OAuth tokens. The linking code must have - allow_account_creation enabled. + allow_account_creation enabled. The first user on a fresh instance is assigned + the super_admin role; later family creators are assigned an admin-capable + role. parameters: [] responses: '200': @@ -4593,85 +4598,6 @@ paths: application/json: schema: "$ref": "#/components/schemas/ErrorResponse" - "/api/v1/insights": - get: - summary: List proactive insights - tags: - - Insights - security: - - apiKeyAuth: [] - responses: - '200': - description: insights listed - content: - application/json: - schema: - "$ref": "#/components/schemas/InsightCollection" - '403': - description: preview features disabled - content: - application/json: - schema: - "$ref": "#/components/schemas/ErrorResponse" - "/api/v1/push_subscriptions": - post: - summary: Register an APNs device token - tags: - - Push Subscriptions - security: - - apiKeyAuth: [] - parameters: [] - responses: - '201': - description: token registered - content: - application/json: - schema: - "$ref": "#/components/schemas/PushSubscription" - '422': - description: invalid or conflicting subscription - content: - application/json: - schema: - "$ref": "#/components/schemas/ErrorResponse" - requestBody: - content: - application/json: - schema: - type: object - required: - - token - - environment - - platform - properties: - token: - type: string - environment: - type: string - enum: - - sandbox - - production - platform: - type: string - enum: - - ios - required: true - "/api/v1/push_subscriptions/{id}": - parameters: - - name: id - in: path - required: true - schema: - type: string - delete: - summary: Unregister an APNs device token - tags: - - Push Subscriptions - security: - - apiKeyAuth: [] - responses: - '204': - description: token unregistered "/api/v1/family_exports": get: summary: Lists family exports @@ -5901,6 +5827,26 @@ paths: type: string description: CSV imports only. Column value that marks an amount as an inflow when using custom_column strategy + "/api/v1/insights": + get: + summary: List proactive insights + tags: + - Insights + security: + - apiKeyAuth: [] + responses: + '200': + description: insights listed + content: + application/json: + schema: + "$ref": "#/components/schemas/InsightCollection" + '403': + description: preview features disabled + content: + application/json: + schema: + "$ref": "#/components/schemas/ErrorResponse" "/api/v1/merchants": get: summary: List merchants @@ -6012,6 +5958,65 @@ paths: application/json: schema: "$ref": "#/components/schemas/ErrorResponse" + "/api/v1/push_subscriptions": + post: + summary: Register an APNs device token + tags: + - Push Subscriptions + security: + - apiKeyAuth: [] + parameters: [] + responses: + '201': + description: token registered + content: + application/json: + schema: + "$ref": "#/components/schemas/PushSubscription" + '422': + description: invalid or conflicting subscription + content: + application/json: + schema: + "$ref": "#/components/schemas/ErrorResponse" + requestBody: + content: + application/json: + schema: + type: object + required: + - token + - environment + - platform + properties: + token: + type: string + environment: + type: string + enum: + - sandbox + - production + platform: + type: string + enum: + - ios + required: true + "/api/v1/push_subscriptions/{id}": + parameters: + - name: id + in: path + required: true + schema: + type: string + delete: + summary: Unregister an APNs device token + tags: + - Push Subscriptions + security: + - apiKeyAuth: [] + responses: + '204': + description: token unregistered "/api/v1/recurring_transactions": get: summary: List recurring transactions diff --git a/spec/requests/api/v1/auth_spec.rb b/spec/requests/api/v1/auth_spec.rb index 38bb01496..4a98dabba 100644 --- a/spec/requests/api/v1/auth_spec.rb +++ b/spec/requests/api/v1/auth_spec.rb @@ -8,6 +8,8 @@ RSpec.describe 'API V1 Auth', type: :request do tags 'Auth' consumes 'application/json' produces 'application/json' + description 'Creates a new user and family. The first user on a fresh instance is assigned ' \ + 'the super_admin role; later family creators are assigned an admin role.' parameter name: :body, in: :body, required: true, schema: { type: :object, properties: { @@ -311,7 +313,10 @@ RSpec.describe 'API V1 Auth', type: :request do tags 'Auth' consumes 'application/json' produces 'application/json' - description 'Creates a new user and family from a previously issued linking code. Links the SSO identity via OidcIdentity, logs the JIT account creation via SsoAuditLog, and issues mobile OAuth tokens. The linking code must have allow_account_creation enabled.' + description 'Creates a new user and family from a previously issued linking code. Links the SSO identity ' \ + 'via OidcIdentity, logs the JIT account creation via SsoAuditLog, and issues mobile OAuth tokens. ' \ + 'The linking code must have allow_account_creation enabled. The first user on a fresh instance is ' \ + 'assigned the super_admin role; later family creators are assigned an admin-capable role.' parameter name: :body, in: :body, required: true, schema: { type: :object, properties: { diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 28a37b876..74d2f6a33 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -3,6 +3,8 @@ require "test_helper" class UserTest < ActiveSupport::TestCase include ActiveJob::TestHelper + uses_transaction :test_first_user_role_lock_makes_concurrent_family_creators_deterministic + def setup @user = users(:family_admin) end @@ -815,6 +817,76 @@ class UserTest < ActiveSupport::TestCase assert_equal "super_admin", User.role_for_new_family_creator(fallback_role: "super_admin") end + test "first user role lock makes concurrent family creators deterministic" do + created_family_ids = Queue.new + + User.connection.disable_referential_integrity { User.delete_all } + first_user_saved = Queue.new + creator_errors = Queue.new + + first_creator = Thread.new do + signaled = false + + ActiveRecord::Base.connection_pool.with_connection do + ActiveRecord::Base.transaction do + family = Family.create! + created_family_ids << family.id + + User.lock_first_user_role! + user = User.create!( + email: "concurrent-first@example.com", + password: user_password_test, + family: family, + role: User.role_for_new_family_creator + ) + first_user_saved << user.id + signaled = true + sleep 0.1 + end + end + rescue StandardError => e + creator_errors << e + first_user_saved << nil unless signaled + end + + first_user_saved.pop + second_creator = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + ActiveRecord::Base.transaction do + family = Family.create! + created_family_ids << family.id + + User.lock_first_user_role! + User.create!( + email: "concurrent-second@example.com", + password: user_password_test, + family: family, + role: User.role_for_new_family_creator + ) + end + end + rescue StandardError => e + creator_errors << e + end + + [ first_creator, second_creator ].each(&:join) + raise creator_errors.pop(true) unless creator_errors.empty? + + assert_equal 1, User.where(role: :super_admin).count + assert User.find_by(email: "concurrent-first@example.com").super_admin? + assert User.find_by(email: "concurrent-second@example.com").admin? + ensure + [ first_creator, second_creator ].compact.each(&:join) + + family_ids = [] + family_ids << created_family_ids.pop(true) until created_family_ids.empty? + + User.connection.disable_referential_integrity do + User.where(email: %w[concurrent-first@example.com concurrent-second@example.com]).delete_all + Family.where(id: family_ids).delete_all if family_ids.any? + end + end + # Preview features preference tests test "preview_features_enabled? defaults to false" do @user.update!(preferences: {})