From 39ab14531a5cbc0ea4d59eb37392fafc9900cfb6 Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Thu, 3 Mar 2022 18:17:02 +0200 Subject: [PATCH] FEATURE: API to create user's associated account (#15737) Discourse users and associated accounts are created or updated when a user logins or connects the account using their account preferences. This new API can be used to create associated accounts and users too, if necessary. --- app/controllers/users_controller.rb | 27 +++- .../admin_detailed_user_serializer.rb | 13 +- app/services/user_updater.rb | 15 ++ spec/lib/discourse_spec.rb | 2 +- spec/requests/admin/users_controller_spec.rb | 9 ++ .../api/schemas/json/admin_user_response.json | 6 +- .../api/schemas/json/user_create_request.json | 3 + .../api/schemas/json/user_update_request.json | 17 +++ .../schemas/json/user_update_response.json | 15 ++ spec/requests/api/users_spec.rb | 26 ++++ spec/requests/users_controller_spec.rb | 131 ++++++++++++++++++ 11 files changed, 260 insertions(+), 4 deletions(-) create mode 100644 spec/requests/api/schemas/json/user_update_request.json create mode 100644 spec/requests/api/schemas/json/user_update_response.json diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index f79e297603e..b3ad88ce425 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -159,6 +159,17 @@ class UsersController < ApplicationController end end + if params[:external_ids]&.is_a?(ActionController::Parameters) && current_user&.admin? && is_api? + attributes[:user_associated_accounts] = [] + + params[:external_ids].each do |provider_name, provider_uid| + authenticator = Discourse.enabled_authenticators.find { |a| a.name == provider_name } + raise Discourse::InvalidParameters.new(:external_ids) if !authenticator&.is_managed? + + attributes[:user_associated_accounts] << { provider_name: provider_name, provider_uid: provider_uid } + end + end + json_result(user, serializer: UserSerializer, additional_errors: [:user_profile, :user_option]) do |u| updater = UserUpdater.new(current_user, user) updater.update(attributes.permit!) @@ -632,6 +643,7 @@ class UsersController < ApplicationController params.require(:username) params.require(:invite_code) if SiteSetting.require_invite_code params.permit(:user_fields) + params.permit(:external_ids) unless SiteSetting.allow_new_registrations return fail_with("login.new_registrations_disabled") @@ -691,6 +703,18 @@ class UsersController < ApplicationController user.custom_fields = fields end + # Handle associated accounts + associations = [] + if params[:external_ids]&.is_a?(ActionController::Parameters) && current_user&.admin? && is_api? + params[:external_ids].each do |provider_name, provider_uid| + authenticator = Discourse.enabled_authenticators.find { |a| a.name == provider_name } + raise Discourse::InvalidParameters.new(:external_ids) if !authenticator&.is_managed? + + association = UserAssociatedAccount.find_or_initialize_by(provider_name: provider_name, provider_uid: provider_uid) + associations << association + end + end + authentication = UserAuthenticator.new(user, session) if !authentication.has_authenticator? && !SiteSetting.enable_local_logins && !(current_user&.admin? && is_api?) @@ -709,11 +733,12 @@ class UsersController < ApplicationController # just assign a password if we have an authenticator and no password # this is the case for Twitter - user.password = SecureRandom.hex if user.password.blank? && authentication.has_authenticator? + user.password = SecureRandom.hex if user.password.blank? && (authentication.has_authenticator? || associations.present?) if user.save authentication.finish activation.finish + associations.each { |a| a.update!(user: user) } user.update_timezone_if_missing(params[:timezone]) secure_session[HONEYPOT_KEY] = nil diff --git a/app/serializers/admin_detailed_user_serializer.rb b/app/serializers/admin_detailed_user_serializer.rb index ed99812b4fa..aab939247c5 100644 --- a/app/serializers/admin_detailed_user_serializer.rb +++ b/app/serializers/admin_detailed_user_serializer.rb @@ -35,7 +35,8 @@ class AdminDetailedUserSerializer < AdminUserSerializer :second_factor_enabled, :can_disable_second_factor, :can_delete_sso_record, - :api_key_count + :api_key_count, + :external_ids has_one :approved_by, serializer: BasicUserSerializer, embed: :objects has_one :suspended_by, serializer: BasicUserSerializer, embed: :objects @@ -145,6 +146,16 @@ class AdminDetailedUserSerializer < AdminUserSerializer object.api_keys.active.count end + def external_ids + external_ids = {} + + object.user_associated_accounts.map do |user_associated_account| + external_ids[user_associated_account.provider_name] = user_associated_account.provider_uid + end + + external_ids + end + def can_delete_sso_record scope.can_delete_sso_record?(object) end diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index 49680afd1af..7c28afd56b2 100644 --- a/app/services/user_updater.rb +++ b/app/services/user_updater.rb @@ -197,6 +197,10 @@ class UserUpdater update_allowed_pm_users(attributes[:allowed_pm_usernames]) end + if attributes.key?(:user_associated_accounts) + updated_associated_accounts(attributes[:user_associated_accounts]) + end + name_changed = user.name_changed? if (saved = (!save_options || user.user_option.save) && (user_notification_schedule.nil? || user_notification_schedule.save) && user_profile.save && user.save) && (name_changed && old_user_name.casecmp(attributes.fetch(:name)) != 0) @@ -265,6 +269,17 @@ class UserUpdater end end + def updated_associated_accounts(associations) + associations.each do |association| + user_associated_account = UserAssociatedAccount.find_or_initialize_by(user_id: user.id, provider_name: association[:provider_name]) + if association[:provider_uid].present? + user_associated_account.update!(provider_uid: association[:provider_uid]) + else + user_associated_account.destroy! + end + end + end + private attr_reader :user, :guardian diff --git a/spec/lib/discourse_spec.rb b/spec/lib/discourse_spec.rb index bdd33cf7df2..5fbdffa6453 100644 --- a/spec/lib/discourse_spec.rb +++ b/spec/lib/discourse_spec.rb @@ -141,7 +141,7 @@ describe Discourse do 'pluginauth' end - def enabled + def enabled? true end end diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index 2356b4ff46e..e4d25e6c247 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -48,6 +48,15 @@ RSpec.describe Admin::UsersController do get "/admin/users/#{user.id}.json" expect(response.status).to eq(200) end + + it 'includes associated accounts' do + user.user_associated_accounts.create!(provider_name: 'pluginauth', provider_uid: 'pluginauth_uid') + + get "/admin/users/#{user.id}.json" + expect(response.status).to eq(200) + expect(response.parsed_body['external_ids'].size).to eq(1) + expect(response.parsed_body['external_ids']['pluginauth']).to eq('pluginauth_uid') + end end context 'a non-existing user' do diff --git a/spec/requests/api/schemas/json/admin_user_response.json b/spec/requests/api/schemas/json/admin_user_response.json index db38d9f99df..06991a559a7 100644 --- a/spec/requests/api/schemas/json/admin_user_response.json +++ b/spec/requests/api/schemas/json/admin_user_response.json @@ -486,6 +486,9 @@ ] } ] + }, + "external_ids": { + "type": "object" } }, "required": [ @@ -547,6 +550,7 @@ "approved_by", "suspended_by", "silenced_by", - "groups" + "groups", + "external_ids" ] } diff --git a/spec/requests/api/schemas/json/user_create_request.json b/spec/requests/api/schemas/json/user_create_request.json index 5b8dc3674f7..ac5fc3265fb 100644 --- a/spec/requests/api/schemas/json/user_create_request.json +++ b/spec/requests/api/schemas/json/user_create_request.json @@ -21,6 +21,9 @@ }, "user_fields[1]": { "type": "boolean" + }, + "external_ids": { + "type": "object" } }, "required": [ diff --git a/spec/requests/api/schemas/json/user_update_request.json b/spec/requests/api/schemas/json/user_update_request.json new file mode 100644 index 00000000000..25dbb89ab39 --- /dev/null +++ b/spec/requests/api/schemas/json/user_update_request.json @@ -0,0 +1,17 @@ +{ + "additionalProperties": false, + "properties": { + "name": { + "type": "string" + }, + "email": { + "type": "string" + }, + "password": { + "type": "string" + }, + "external_ids": { + "type": "object" + } + } +} diff --git a/spec/requests/api/schemas/json/user_update_response.json b/spec/requests/api/schemas/json/user_update_response.json new file mode 100644 index 00000000000..486ca6dcc72 --- /dev/null +++ b/spec/requests/api/schemas/json/user_update_response.json @@ -0,0 +1,15 @@ +{ + "additionalProperties": false, + "properties": { + "success": { + "type": "string" + }, + "user": { + "type": "object" + } + }, + "required": [ + "success", + "user" + ] +} diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 7c1cf361fe9..4353b1364d8 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -71,6 +71,32 @@ describe 'users' do end end end + + put 'Update a user' do + tags 'Users' + operationId 'updateUser' + consumes 'application/json' + + parameter name: 'Api-Key', in: :header, type: :string, required: true + parameter name: 'Api-Username', in: :header, type: :string, required: true + expected_request_schema = load_spec_schema('user_update_request') + parameter name: :username, in: :path, type: :string, required: true + parameter name: :params, in: :body, schema: expected_request_schema + + produces 'application/json' + response '200', 'user updated' do + expected_response_schema = load_spec_schema('user_update_response') + schema expected_response_schema + + let(:username) { Fabricate(:user).username } + let(:params) { { 'name' => 'user' } } + + it_behaves_like "a JSON endpoint", 200 do + let(:expected_response_schema) { expected_response_schema } + let(:expected_request_schema) { expected_request_schema } + end + end + end end path '/u/by-external/{external_id}.json' do diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 78de7a391b8..2648d718b9b 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -648,6 +648,69 @@ describe UsersController do expect(response.status).to eq(200) end end + + context 'with external_ids' do + fab!(:api_key, refind: false) { Fabricate(:api_key, user: admin) } + + let(:plugin_auth_provider) do + authenticator_class = Class.new(Auth::ManagedAuthenticator) do + def name + 'pluginauth' + end + + def enabled? + true + end + end + + provider = Auth::AuthProvider.new + provider.authenticator = authenticator_class.new + provider + end + + before do + DiscoursePluginRegistry.register_auth_provider(plugin_auth_provider) + end + + after do + DiscoursePluginRegistry.reset! + end + + it 'creates User record' do + params = { + username: 'foobar', + email: 'test@example.com', + external_ids: { 'pluginauth' => 'pluginauth_uid' }, + } + + expect { post "/u.json", params: params, headers: { HTTP_API_KEY: api_key.key } } + .to change { UserAssociatedAccount.count }.by(1) + .and change { User.count }.by(1) + + expect(response.status).to eq(200) + + user = User.last + user_associated_account = UserAssociatedAccount.last + + expect(user.username).to eq('foobar') + expect(user.email).to eq('test@example.com') + expect(user.user_associated_account_ids).to contain_exactly(user_associated_account.id) + expect(user_associated_account.provider_name).to eq('pluginauth') + expect(user_associated_account.provider_uid).to eq('pluginauth_uid') + expect(user_associated_account.user_id).to eq(user.id) + end + + it 'returns error if external ID provider does not exist' do + params = { + username: 'foobar', + email: 'test@example.com', + external_ids: { 'pluginauth2' => 'pluginauth_uid' }, + } + + post "/u.json", params: params, headers: { HTTP_API_KEY: api_key.key } + expect(response.status).to eq(400) + end + end end context 'when creating a non active user (unconfirmed email)' do @@ -2231,6 +2294,74 @@ describe UsersController do end end end + + context 'with external_ids' do + fab!(:api_key, refind: false) { Fabricate(:api_key, user: admin) } + + let(:plugin_auth_provider) do + authenticator_class = Class.new(Auth::ManagedAuthenticator) do + def name + 'pluginauth' + end + + def enabled? + true + end + end + + provider = Auth::AuthProvider.new + provider.authenticator = authenticator_class.new + provider + end + + before do + DiscoursePluginRegistry.register_auth_provider(plugin_auth_provider) + end + + after do + DiscoursePluginRegistry.reset! + end + + it 'can create UserAssociatedAccount records' do + params = { + external_ids: { 'pluginauth' => 'pluginauth_uid' }, + } + + expect { put "/u/#{user.username}.json", params: params, headers: { HTTP_API_KEY: api_key.key } } + .to change { UserAssociatedAccount.count }.by(1) + + expect(response.status).to eq(200) + + user_associated_account = UserAssociatedAccount.last + expect(user.reload.user_associated_account_ids).to contain_exactly(user_associated_account.id) + expect(user_associated_account.provider_name).to eq('pluginauth') + expect(user_associated_account.provider_uid).to eq('pluginauth_uid') + expect(user_associated_account.user_id).to eq(user.id) + end + + it 'can destroy UserAssociatedAccount records' do + user.user_associated_accounts.create!(provider_name: 'pluginauth', provider_uid: 'pluginauth_uid') + + params = { + external_ids: { 'pluginauth' => nil }, + } + + expect { put "/u/#{user.username}.json", params: params, headers: { HTTP_API_KEY: api_key.key } } + .to change { UserAssociatedAccount.count }.by(-1) + + expect(response.status).to eq(200) + expect(user.reload.user_associated_account_ids).to be_blank + end + + it 'returns error if external ID provider does not exist' do + params = { + external_ids: { 'pluginauth2' => 'pluginauth_uid' }, + } + + put "/u/#{user.username}.json", params: params, headers: { HTTP_API_KEY: api_key.key } + expect(response.status).to eq(400) + end + end end describe '#badge_title' do