From 5d2fc6de321ac556ded72e5a8fa9fcf47d172e16 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 24 Dec 2018 19:12:38 +0100 Subject: [PATCH] Add REST API for creating an account (#9572) * Add REST API for creating an account The method is available to apps with a token obtained via the client credentials grant. It creates a user and account records, as well as an access token for the app that initiated the request. The user is unconfirmed, and an e-mail is sent as usual. The method returns the access token, which the app should save for later. The REST API is not available to users with unconfirmed accounts, so the app must be smart to wait for the user to click a link in their e-mail inbox. The method is rate-limited by IP to 5 requests per 30 minutes. * Redirect users back to app from confirmation if they were created with an app * Add tests * Return 403 on the method if registrations are not open * Require agreement param to be true in the API when creating an account --- app/controllers/api/base_controller.rb | 2 +- app/controllers/api/v1/accounts_controller.rb | 26 ++++++++++-- .../auth/confirmations_controller.rb | 10 ++++- .../auth/registrations_controller.rb | 1 + app/models/user.rb | 7 +++- app/services/app_sign_up_service.rb | 23 +++++++++++ .../confirmation_instructions.html.haml | 8 +++- .../confirmation_instructions.text.erb | 2 +- config/initializers/rack_attack.rb | 4 ++ config/locales/devise.en.yml | 1 + config/routes.rb | 2 +- ..._add_created_by_application_id_to_users.rb | 8 ++++ db/schema.rb | 5 ++- lib/mastodon/accounts_cli.rb | 2 +- .../api/v1/accounts_controller_spec.rb | 34 +++++++++++++++ spec/fabricators/user_fabricator.rb | 1 + spec/models/user_spec.rb | 14 +++---- spec/services/app_sign_up_service_spec.rb | 41 +++++++++++++++++++ 18 files changed, 171 insertions(+), 20 deletions(-) create mode 100644 app/services/app_sign_up_service.rb create mode 100644 db/migrate/20181219235220_add_created_by_application_id_to_users.rb create mode 100644 spec/services/app_sign_up_service_spec.rb diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index ac8de5fc0f..2bf8e82db8 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -68,7 +68,7 @@ class Api::BaseController < ApplicationController end def require_user! - if current_user && !current_user.disabled? + if current_user && !current_user.disabled? && current_user.confirmed? set_user_activity elsif current_user render json: { error: 'Your login is currently disabled' }, status: 403 diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index f711c46767..6e4084c4e3 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -1,14 +1,16 @@ # frozen_string_literal: true class Api::V1::AccountsController < Api::BaseController - before_action -> { authorize_if_got_token! :read, :'read:accounts' }, except: [:follow, :unfollow, :block, :unblock, :mute, :unmute] + before_action -> { authorize_if_got_token! :read, :'read:accounts' }, except: [:create, :follow, :unfollow, :block, :unblock, :mute, :unmute] before_action -> { doorkeeper_authorize! :follow, :'write:follows' }, only: [:follow, :unfollow] before_action -> { doorkeeper_authorize! :follow, :'write:mutes' }, only: [:mute, :unmute] before_action -> { doorkeeper_authorize! :follow, :'write:blocks' }, only: [:block, :unblock] + before_action -> { doorkeeper_authorize! :write, :'write:accounts' }, only: [:create] - before_action :require_user!, except: [:show] - before_action :set_account + before_action :require_user!, except: [:show, :create] + before_action :set_account, except: [:create] before_action :check_account_suspension, only: [:show] + before_action :check_enabled_registrations, only: [:create] respond_to :json @@ -16,6 +18,16 @@ class Api::V1::AccountsController < Api::BaseController render json: @account, serializer: REST::AccountSerializer end + def create + token = AppSignUpService.new.call(doorkeeper_token.application, account_params) + response = Doorkeeper::OAuth::TokenResponse.new(token) + + headers.merge!(response.headers) + + self.response_body = Oj.dump(response.body) + self.status = response.status + end + def follow FollowService.new.call(current_user.account, @account, reblogs: truthy_param?(:reblogs)) @@ -62,4 +74,12 @@ class Api::V1::AccountsController < Api::BaseController def check_account_suspension gone if @account.suspended? end + + def account_params + params.permit(:username, :email, :password, :agreement) + end + + def check_enabled_registrations + forbidden if single_user_mode? || !Setting.open_registrations + end end diff --git a/app/controllers/auth/confirmations_controller.rb b/app/controllers/auth/confirmations_controller.rb index 7af9cbe81d..c28c7471c0 100644 --- a/app/controllers/auth/confirmations_controller.rb +++ b/app/controllers/auth/confirmations_controller.rb @@ -6,9 +6,9 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController before_action :set_body_classes before_action :set_user, only: [:finish_signup] - # GET/PATCH /users/:id/finish_signup def finish_signup return unless request.patch? && params[:user] + if @user.update(user_params) @user.skip_reconfirmation! bypass_sign_in(@user) @@ -31,4 +31,12 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController def user_params params.require(:user).permit(:email) end + + def after_confirmation_path_for(_resource_name, user) + if user.created_by_application && truthy_param?(:redirect_to_app) + user.created_by_application.redirect_uri + else + super + end + end end diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index 088832be3c..f2a832542f 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -26,6 +26,7 @@ class Auth::RegistrationsController < Devise::RegistrationsController resource.locale = I18n.locale resource.invite_code = params[:invite_code] if resource.invite_code.blank? + resource.agreement = true resource.build_account if resource.account.nil? end diff --git a/app/models/user.rb b/app/models/user.rb index 44e0d11139..77e48ed4b9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -36,6 +36,7 @@ # invite_id :bigint(8) # remember_token :string # chosen_languages :string is an Array +# created_by_application_id :bigint(8) # class User < ApplicationRecord @@ -66,6 +67,7 @@ class User < ApplicationRecord belongs_to :account, inverse_of: :user belongs_to :invite, counter_cache: :uses, optional: true + belongs_to :created_by_application, class_name: 'Doorkeeper::Application', optional: true accepts_nested_attributes_for :account has_many :applications, class_name: 'Doorkeeper::Application', as: :owner @@ -74,6 +76,7 @@ class User < ApplicationRecord validates :locale, inclusion: I18n.available_locales.map(&:to_s), if: :locale? validates_with BlacklistedEmailValidator, if: :email_changed? validates_with EmailMxValidator, if: :validate_email_dns? + validates :agreement, acceptance: { allow_nil: false, accept: [true, 'true', '1'] }, on: :create scope :recent, -> { order(id: :desc) } scope :admins, -> { where(admin: true) } @@ -294,7 +297,7 @@ class User < ApplicationRecord end if resource.blank? - resource = new(email: attributes[:email]) + resource = new(email: attributes[:email], agreement: true) if Devise.check_at_sign && !resource[:email].index('@') resource[:email] = Rpam2.getenv(resource.find_pam_service, attributes[:email], attributes[:password], 'email', false) resource[:email] = "#{attributes[:email]}@#{resource.find_pam_suffix}" unless resource[:email] @@ -307,7 +310,7 @@ class User < ApplicationRecord resource = joins(:account).find_by(accounts: { username: attributes[Devise.ldap_uid.to_sym].first }) if resource.blank? - resource = new(email: attributes[:mail].first, account_attributes: { username: attributes[Devise.ldap_uid.to_sym].first }) + resource = new(email: attributes[:mail].first, agreement: true, account_attributes: { username: attributes[Devise.ldap_uid.to_sym].first }) resource.ldap_setup(attributes) end diff --git a/app/services/app_sign_up_service.rb b/app/services/app_sign_up_service.rb new file mode 100644 index 0000000000..1878587e80 --- /dev/null +++ b/app/services/app_sign_up_service.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class AppSignUpService < BaseService + def call(app, params) + return unless allowed_registrations? + + user_params = params.slice(:email, :password, :agreement) + account_params = params.slice(:username) + user = User.create!(user_params.merge(created_by_application: app, password_confirmation: user_params[:password], account_attributes: account_params)) + + Doorkeeper::AccessToken.create!(application: app, + resource_owner_id: user.id, + scopes: app.scopes, + expires_in: Doorkeeper.configuration.access_token_expires_in, + use_refresh_token: Doorkeeper.configuration.refresh_token_enabled?) + end + + private + + def allowed_registrations? + Setting.open_registrations && !Rails.configuration.x.single_user_mode + end +end diff --git a/app/views/user_mailer/confirmation_instructions.html.haml b/app/views/user_mailer/confirmation_instructions.html.haml index 1f088a16f4..f75f7529a8 100644 --- a/app/views/user_mailer/confirmation_instructions.html.haml +++ b/app/views/user_mailer/confirmation_instructions.html.haml @@ -55,8 +55,12 @@ %tbody %tr %td.button-primary - = link_to confirmation_url(@resource, confirmation_token: @token) do - %span= t 'devise.mailer.confirmation_instructions.action' + - if @resource.created_by_application + = link_to confirmation_url(@resource, confirmation_token: @token, redirect_to_app: 'true') do + %span= t 'devise.mailer.confirmation_instructions.action_with_app', app: @resource.created_by_application.name + - else + = link_to confirmation_url(@resource, confirmation_token: @token) do + %span= t 'devise.mailer.confirmation_instructions.action' %table.email-table{ cellspacing: 0, cellpadding: 0 } %tbody diff --git a/app/views/user_mailer/confirmation_instructions.text.erb b/app/views/user_mailer/confirmation_instructions.text.erb index e01eecb27e..65b4626c66 100644 --- a/app/views/user_mailer/confirmation_instructions.text.erb +++ b/app/views/user_mailer/confirmation_instructions.text.erb @@ -4,7 +4,7 @@ <%= t 'devise.mailer.confirmation_instructions.explanation', host: site_hostname %> -=> <%= confirmation_url(@resource, confirmation_token: @token) %> +=> <%= confirmation_url(@resource, confirmation_token: @token, redirect_to_app: @resource.created_by_application ? 'true' : nil) %> <%= strip_tags(t('devise.mailer.confirmation_instructions.extra_html', terms_path: about_more_url, policy_path: terms_url)) %> diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 8756b8fbf0..35302e37b1 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -57,6 +57,10 @@ class Rack::Attack req.authenticated_user_id if req.post? && req.path.start_with?('/api/v1/media') end + throttle('throttle_api_sign_up', limit: 5, period: 30.minutes) do |req| + req.ip if req.post? && req.path == '/api/v1/accounts' + end + throttle('protected_paths', limit: 25, period: 5.minutes) do |req| req.ip if req.post? && req.path =~ PROTECTED_PATHS_REGEX end diff --git a/config/locales/devise.en.yml b/config/locales/devise.en.yml index 20938e47b6..bd0642b251 100644 --- a/config/locales/devise.en.yml +++ b/config/locales/devise.en.yml @@ -18,6 +18,7 @@ en: mailer: confirmation_instructions: action: Verify email address + action_with_app: Confirm and return to %{app} explanation: You have created an account on %{host} with this email address. You are one click away from activating it. If this wasn't you, please ignore this email. extra_html: Please also check out the rules of the instance and our terms of service. subject: 'Mastodon: Confirmation instructions for %{instance}' diff --git a/config/routes.rb b/config/routes.rb index 7723a08af9..808bb5acd6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -336,7 +336,7 @@ Rails.application.routes.draw do resources :relationships, only: :index end - resources :accounts, only: [:show] do + resources :accounts, only: [:create, :show] do resources :statuses, only: :index, controller: 'accounts/statuses' resources :followers, only: :index, controller: 'accounts/follower_accounts' resources :following, only: :index, controller: 'accounts/following_accounts' diff --git a/db/migrate/20181219235220_add_created_by_application_id_to_users.rb b/db/migrate/20181219235220_add_created_by_application_id_to_users.rb new file mode 100644 index 0000000000..17ce900af3 --- /dev/null +++ b/db/migrate/20181219235220_add_created_by_application_id_to_users.rb @@ -0,0 +1,8 @@ +class AddCreatedByApplicationIdToUsers < ActiveRecord::Migration[5.2] + disable_ddl_transaction! + + def change + add_reference :users, :created_by_application, foreign_key: { to_table: 'oauth_applications', on_delete: :nullify }, index: false + add_index :users, :created_by_application_id, algorithm: :concurrently + end +end diff --git a/db/schema.rb b/db/schema.rb index 51a7b5e749..e47960b169 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.define(version: 2018_12_13_185533) do +ActiveRecord::Schema.define(version: 2018_12_19_235220) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -637,8 +637,10 @@ ActiveRecord::Schema.define(version: 2018_12_13_185533) do t.bigint "invite_id" t.string "remember_token" t.string "chosen_languages", array: true + t.bigint "created_by_application_id" t.index ["account_id"], name: "index_users_on_account_id" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true + t.index ["created_by_application_id"], name: "index_users_on_created_by_application_id" t.index ["email"], name: "index_users_on_email", unique: true t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true end @@ -730,6 +732,7 @@ ActiveRecord::Schema.define(version: 2018_12_13_185533) do add_foreign_key "subscriptions", "accounts", name: "fk_9847d1cbb5", on_delete: :cascade add_foreign_key "users", "accounts", name: "fk_50500f500d", on_delete: :cascade add_foreign_key "users", "invites", on_delete: :nullify + add_foreign_key "users", "oauth_applications", column: "created_by_application_id", on_delete: :nullify add_foreign_key "web_push_subscriptions", "oauth_access_tokens", column: "access_token_id", on_delete: :cascade add_foreign_key "web_push_subscriptions", "users", on_delete: :cascade add_foreign_key "web_settings", "users", name: "fk_11910667b2", on_delete: :cascade diff --git a/lib/mastodon/accounts_cli.rb b/lib/mastodon/accounts_cli.rb index b219682232..bbda244ead 100644 --- a/lib/mastodon/accounts_cli.rb +++ b/lib/mastodon/accounts_cli.rb @@ -73,7 +73,7 @@ module Mastodon def create(username) account = Account.new(username: username) password = SecureRandom.hex - user = User.new(email: options[:email], password: password, admin: options[:role] == 'admin', moderator: options[:role] == 'moderator', confirmed_at: Time.now.utc) + user = User.new(email: options[:email], password: password, agreement: true, admin: options[:role] == 'admin', moderator: options[:role] == 'moderator', confirmed_at: options[:confirmed] ? Time.now.utc : nil) if options[:reattach] account = Account.find_local(username) || Account.new(username: username) diff --git a/spec/controllers/api/v1/accounts_controller_spec.rb b/spec/controllers/api/v1/accounts_controller_spec.rb index c506fb5f01..f5f65c000e 100644 --- a/spec/controllers/api/v1/accounts_controller_spec.rb +++ b/spec/controllers/api/v1/accounts_controller_spec.rb @@ -19,6 +19,40 @@ RSpec.describe Api::V1::AccountsController, type: :controller do end end + describe 'POST #create' do + let(:app) { Fabricate(:application) } + let(:token) { Doorkeeper::AccessToken.find_or_create_for(app, nil, 'read write', nil, false) } + let(:agreement) { nil } + + before do + post :create, params: { username: 'test', password: '12345678', email: 'hello@world.tld', agreement: agreement } + end + + context 'given truthy agreement' do + let(:agreement) { 'true' } + + it 'returns http success' do + expect(response).to have_http_status(200) + end + + it 'returns a new access token as JSON' do + expect(body_as_json[:access_token]).to_not be_blank + end + + it 'creates a user' do + user = User.find_by(email: 'hello@world.tld') + expect(user).to_not be_nil + expect(user.created_by_application_id).to eq app.id + end + end + + context 'given no agreement' do + it 'returns http unprocessable entity' do + expect(response).to have_http_status(422) + end + end + end + describe 'GET #show' do let(:scopes) { 'read:accounts' } diff --git a/spec/fabricators/user_fabricator.rb b/spec/fabricators/user_fabricator.rb index 7dfbdb52d7..8f59565016 100644 --- a/spec/fabricators/user_fabricator.rb +++ b/spec/fabricators/user_fabricator.rb @@ -3,4 +3,5 @@ Fabricator(:user) do email { sequence(:email) { |i| "#{i}#{Faker::Internet.email}" } } password "123456789" confirmed_at { Time.zone.now } + agreement true end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c829195972..856254ce4b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -106,19 +106,19 @@ RSpec.describe User, type: :model do end it 'should allow a non-blacklisted user to be created' do - user = User.new(email: 'foo@example.com', account: account, password: password) + user = User.new(email: 'foo@example.com', account: account, password: password, agreement: true) expect(user.valid?).to be_truthy end it 'should not allow a blacklisted user to be created' do - user = User.new(email: 'foo@mvrht.com', account: account, password: password) + user = User.new(email: 'foo@mvrht.com', account: account, password: password, agreement: true) expect(user.valid?).to be_falsey end it 'should not allow a subdomain blacklisted user to be created' do - user = User.new(email: 'foo@mvrht.com.topdomain.tld', account: account, password: password) + user = User.new(email: 'foo@mvrht.com.topdomain.tld', account: account, password: password, agreement: true) expect(user.valid?).to be_falsey end @@ -210,17 +210,17 @@ RSpec.describe User, type: :model do end it 'should not allow a user to be created unless they are whitelisted' do - user = User.new(email: 'foo@example.com', account: account, password: password) + user = User.new(email: 'foo@example.com', account: account, password: password, agreement: true) expect(user.valid?).to be_falsey end it 'should allow a user to be created if they are whitelisted' do - user = User.new(email: 'foo@mastodon.space', account: account, password: password) + user = User.new(email: 'foo@mastodon.space', account: account, password: password, agreement: true) expect(user.valid?).to be_truthy end it 'should not allow a user with a whitelisted top domain as subdomain in their email address to be created' do - user = User.new(email: 'foo@mastodon.space.userdomain.com', account: account, password: password) + user = User.new(email: 'foo@mastodon.space.userdomain.com', account: account, password: password, agreement: true) expect(user.valid?).to be_falsey end @@ -242,7 +242,7 @@ RSpec.describe User, type: :model do it_behaves_like 'Settings-extended' do def create! - User.create!(account: Fabricate(:account), email: 'foo@mastodon.space', password: 'abcd1234') + User.create!(account: Fabricate(:account), email: 'foo@mastodon.space', password: 'abcd1234', agreement: true) end def fabricate diff --git a/spec/services/app_sign_up_service_spec.rb b/spec/services/app_sign_up_service_spec.rb new file mode 100644 index 0000000000..d480df3480 --- /dev/null +++ b/spec/services/app_sign_up_service_spec.rb @@ -0,0 +1,41 @@ +require 'rails_helper' + +RSpec.describe AppSignUpService, type: :service do + let(:app) { Fabricate(:application, scopes: 'read write') } + let(:good_params) { { username: 'alice', password: '12345678', email: 'good@email.com', agreement: true } } + + subject { described_class.new } + + describe '#call' do + it 'returns nil when registrations are closed' do + Setting.open_registrations = false + expect(subject.call(app, good_params)).to be_nil + end + + it 'raises an error when params are missing' do + expect { subject.call(app, {}) }.to raise_error ActiveRecord::RecordInvalid + end + + it 'creates an unconfirmed user with access token' do + access_token = subject.call(app, good_params) + expect(access_token).to_not be_nil + user = User.find_by(id: access_token.resource_owner_id) + expect(user).to_not be_nil + expect(user.confirmed?).to be false + end + + it 'creates access token with the app\'s scopes' do + access_token = subject.call(app, good_params) + expect(access_token).to_not be_nil + expect(access_token.scopes.to_s).to eq 'read write' + end + + it 'creates an account' do + access_token = subject.call(app, good_params) + expect(access_token).to_not be_nil + user = User.find_by(id: access_token.resource_owner_id) + expect(user).to_not be_nil + expect(user.account).to_not be_nil + end + end +end