diff --git a/NEWS.md b/NEWS.md index 47b67931fb3d..19736c1d85bf 100644 --- a/NEWS.md +++ b/NEWS.md @@ -9,7 +9,8 @@ Development * Load config files as ERB templates to allow reading ENV values ([15881](https://github.com/CartoDB/cartodb/pull/15881)) ### Bug fixes / enhancements -- Add DO geography key variables [#15882](https://github.com/CartoDB/cartodb/pull/15882) +* Add DO geography key variables [#15882](https://github.com/CartoDB/cartodb/pull/15882) +* Migrate `ClientApplication` model to `ActiveRecord` [#15886](https://github.com/CartoDB/cartodb/pull/15886) 4.42.0 (2020-09-28) ------------------- diff --git a/app/controllers/admin/client_applications_controller.rb b/app/controllers/admin/client_applications_controller.rb index 1ebbae1e82f4..30cc299ba05c 100644 --- a/app/controllers/admin/client_applications_controller.rb +++ b/app/controllers/admin/client_applications_controller.rb @@ -55,6 +55,7 @@ def regenerate_api_key def regenerate_oauth @client_application = current_user.client_application return if request.get? + current_user.reset_client_application! redirect_to CartoDB.url(self, 'oauth_credentials', params: { type: 'oauth' }, user: current_user), diff --git a/app/models/carto/client_application.rb b/app/models/carto/client_application.rb index fff772965b3c..07fc79932e17 100644 --- a/app/models/carto/client_application.rb +++ b/app/models/carto/client_application.rb @@ -1,11 +1,49 @@ require 'active_record' +require 'oauth' +require_dependency 'cartodb_config_utils' +require_dependency 'carto/domain_patcher_request_proxy' module Carto class ClientApplication < ActiveRecord::Base + extend CartoDB::ConfigUtils + belongs_to :user, class_name: Carto::User has_many :oauth_tokens, class_name: Carto::OauthToken, dependent: :destroy has_many :access_tokens, -> { where(type: 'AccessToken') }, class_name: Carto::OauthToken, dependent: :destroy + before_create :initialize_entity + + def oauth_server + # check if this is used + @oauth_server ||= OAuth::Server.new('http://your.site') + end + + def self.find_token(token_key) + return nil if token_key.nil? + + token = Carto::RequestToken.find_by(token: token_key) || Carto::AccessToken.find_by(token: token_key) + token&.authorized? ? token : nil + end + + def self.verify_request(request, options = {}, &block) + value = OAuth::Signature.build(request, options, &block).verify + if !value && !cartodb_com_hosted? + # Validation failed, try to see if it has been signed for cartodb.com + cartodb_request = Carto::DomainPatcherRequestProxy.new(request, options) + value = OAuth::Signature.build(cartodb_request, options, &block).verify + end + value + rescue OAuth::Signature::UnknownSignatureMethod + false + end + + private + + def initialize_entity + self.key = OAuth::Helper.generate_key(40)[0, 40] + self.secret = OAuth::Helper.generate_key(40)[0, 40] + end + end end diff --git a/app/models/carto/helpers/user_commons.rb b/app/models/carto/helpers/user_commons.rb index b4396aec5e00..e718bae766ce 100644 --- a/app/models/carto/helpers/user_commons.rb +++ b/app/models/carto/helpers/user_commons.rb @@ -381,4 +381,14 @@ def active_record_organization end end end + + def new_client_application + Carto::ClientApplication.create!(user_id: id) + end + + def reset_client_application! + client_application&.destroy + Carto::ClientApplication.create!(user_id: id) + end + end diff --git a/app/models/carto/user.rb b/app/models/carto/user.rb index 351ae244e78a..db4204bf96fd 100644 --- a/app/models/carto/user.rb +++ b/app/models/carto/user.rb @@ -51,7 +51,7 @@ class Carto::User < ActiveRecord::Base has_many :permissions, inverse_of: :owner, foreign_key: :owner_id has_many :connector_configurations, inverse_of: :user, dependent: :destroy - has_many :client_applications, class_name: Carto::ClientApplication, dependent: :destroy + has_one :client_application, class_name: Carto::ClientApplication, dependent: :destroy has_many :tokens, class_name: Carto::OauthToken, dependent: :destroy has_many :users_group, dependent: :destroy, class_name: Carto::UsersGroup @@ -95,11 +95,6 @@ def carto_user; self end after_destroy { rate_limit.destroy_completely(self) if rate_limit } after_destroy :invalidate_varnish_cache - # Compatibility with ::User, where the association is defined as one_to_one - def client_application - client_applications.first - end - # Auto creates notifications on first access def static_notifications_with_creation static_notifications_without_creation || build_static_notifications(user: self, notifications: {}) diff --git a/app/models/client_application.rb b/app/models/client_application.rb deleted file mode 100644 index 93ed3efd9704..000000000000 --- a/app/models/client_application.rb +++ /dev/null @@ -1,83 +0,0 @@ -require 'oauth' - -class DomainPatcherRequestProxy < OAuth::RequestProxy::RackRequest - def uri - super.sub('carto.com', 'cartodb.com') - end -end - -class ClientApplication < Sequel::Model - extend CartoDB::ConfigUtils - - attr_accessor :token_callback_url - - def tokens - Carto::OauthToken.where(client_application_id: id) - end - - def access_tokens - tokens.where(type: 'AccessToken') - end - - def oauth_tokens - tokens - end - - def self.find_token(token_key) - return nil if token_key.nil? - - token = Carto::RequestToken.find_by(token: token_key) || Carto::AccessToken.find_by(token: token_key) - token && token.authorized? ? token : nil - end - - def self.find_by_key(key) - first(:key => key) - end - - def user - ::User[user_id] - end - - def user=(value) - set(:user_id => value.id) - end - - def self.verify_request(request, options = {}, &block) - value = OAuth::Signature.build(request, options, &block).verify - if !value && !cartodb_com_hosted? - # Validation failed, try to see if it has been signed for cartodb.com - cartodb_request = DomainPatcherRequestProxy.new(request, options) - value = OAuth::Signature.build(cartodb_request, options, &block).verify - end - value - rescue OAuth::Signature::UnknownSignatureMethod - false - end - - def oauth_server - @oauth_server ||= OAuth::Server.new("http://your.site") - end - - def credentials - @oauth_client ||= OAuth::Consumer.new(key, secret) - end - - # If your application requires passing in extra parameters handle it here - def create_request_token(params={}) - Carto::RequestToken.create :client_application => self, :callback_url=>self.token_callback_url - end - - def before_create - self.key = OAuth::Helper.generate_key(40)[0,40] - self.secret = OAuth::Helper.generate_key(40)[0,40] - self.created_at = Time.now - end - - def before_save - self.updated_at = Time.now - end - - def before_destroy - oauth_tokens.map(&:destroy) - end -end diff --git a/app/models/user.rb b/app/models/user.rb index 1160a76c7846..8e9e22ebda22 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -38,7 +38,6 @@ class User < Sequel::Model self.strict_param_setting = false - one_to_one :client_application one_to_many :synchronization_oauths one_to_many :maps one_to_many :assets @@ -62,7 +61,7 @@ def self_feature_flags many_through_many :groups, [[:users_groups, :user_id, :group_id]] # Sequel setup & plugins - plugin :association_dependencies, :client_application => :destroy, :synchronization_oauths => :destroy + plugin :association_dependencies, synchronization_oauths: :destroy plugin :validation_helpers plugin :json_serializer plugin :dirty @@ -437,7 +436,7 @@ def before_destroy(skip_table_drop: false) assign_search_tweets_to_organization_owner - ClientApplication.where(user_id: id).each(&:destroy) + Carto::ClientApplication.where(user_id: id).destroy_all rescue StandardError => exception error_happened = true log_error(message: 'Error destroying user', current_user: self, exception: exception) @@ -997,13 +996,6 @@ def get_last_ip_address $users_metadata.HMGET(key, 'last_ip_address').first end - def reset_client_application! - if client_application - client_application.destroy - end - ClientApplication.create(:user_id => self.id) - end - def self.find_with_custom_fields(user_id) ::User.filter(:id => user_id).select(:id,:email,:username,:crypted_password,:database_name,:admin).first end @@ -1236,8 +1228,8 @@ def has_organization_enabled? end end - def create_client_application - ClientApplication.create(:user_id => self.id) + def client_application + Carto::ClientApplication.find_by(user_id: id) end ## User's databases setup methods @@ -1245,7 +1237,7 @@ def setup_user return if disabled? db_service.set_database_name - create_client_application + new_client_application if self.has_organization_enabled? db_service.new_organization_user_main_db_setup else diff --git a/app/services/carto/user_metadata_export_service.rb b/app/services/carto/user_metadata_export_service.rb index 71d6c19625a9..da14498d9df5 100644 --- a/app/services/carto/user_metadata_export_service.rb +++ b/app/services/carto/user_metadata_export_service.rb @@ -87,15 +87,22 @@ def build_search_tweets_from_hash_export(exported_hash) end def save_imported_user(user) + # Keep client_application imported timestamps + if user.client_application + import_app_updated_at = user.client_application.updated_at + import_app_created_at = user.client_application.created_at + end + user.save! ::User[user.id].after_save - client_application = user.client_applications.first - if client_application - client_application.access_tokens.each do |t| - t.update!(type: 'AccessToken') - end + return unless user.client_application + + user.client_application.access_tokens.each do |t| + t.update!(type: 'AccessToken') end + + user.client_application.update_columns(created_at: import_app_created_at, updated_at: import_app_updated_at) end def save_imported_search_tweet(search_tweet, user) @@ -146,7 +153,7 @@ def build_user_from_hash(exported_user) user.connector_configurations = build_connector_configurations_from_hash(exported_user[:connector_configurations]) - user.client_applications = build_client_applications_from_hash(exported_user[:client_application]) + user.client_application = build_client_application_from_hash(exported_user[:client_application]) user.oauth_app_users = build_oauth_app_users_from_hash(exported_user[:oauth_app_users]) @@ -222,23 +229,24 @@ def build_oauth_token_fom_hash(exported_oauth_token) ) end - def build_client_applications_from_hash(client_app_hash) - return [] unless client_app_hash + def build_client_application_from_hash(client_app_hash) + return unless client_app_hash - client_application = Carto::ClientApplication.new( + client_application = Carto::ClientApplication.create( name: client_app_hash[:name], url: client_app_hash[:url], support_url: client_app_hash[:support_url], callback_url: client_app_hash[:callback_url], - key: client_app_hash[:key], - secret: client_app_hash[:secret], - created_at: client_app_hash[:created_at], - updated_at: client_app_hash[:updated_at], oauth_tokens: client_app_hash[:oauth_tokens].map { |t| build_oauth_token_fom_hash(t) }, - access_tokens: client_app_hash[:access_tokens].map { |t| build_oauth_token_fom_hash(t) } + access_tokens: client_app_hash[:access_tokens].map { |t| build_oauth_token_fom_hash(t) }, + user_id: client_app_hash[:user_id] ) - - [client_application] + # Overwrite fields that were created with ORM lifecycle callbacks + client_application.key = client_app_hash[:key] + client_application.secret = client_app_hash[:secret] + client_application.created_at = client_app_hash[:created_at] + client_application.updated_at = client_app_hash[:updated_at] + client_application end def build_oauth_app_users_from_hash(oauth_app_users) @@ -371,7 +379,8 @@ def export_client_application(app) created_at: app.created_at, updated_at: app.updated_at, oauth_tokens: app.oauth_tokens.reject { |t| a_t_tokens.include?(t.token) }.map { |ot| export_oauth_token(ot) }, - access_tokens: app.access_tokens.map { |ot| export_oauth_token(ot) } + access_tokens: app.access_tokens.map { |ot| export_oauth_token(ot) }, + user_id: app.user_id } end diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb index 6739aac0717f..1e05bb3c4d20 100644 --- a/config/initializers/warden.rb +++ b/config/initializers/warden.rb @@ -181,8 +181,8 @@ def authenticate! # WARNING: The following code is a modified copy of the oauth10_token method from # oauth-plugin-0.4.0.pre4/lib/oauth/controllers/application_controller_methods.rb # It also checks token class like does the oauth10_access_token method of that same file - if ClientApplication.verify_request(request) do |request_proxy| - @oauth_token = ClientApplication.find_token(request_proxy.token) + if Carto::ClientApplication.verify_request(request) do |request_proxy| + @oauth_token = Carto::ClientApplication.find_token(request_proxy.token) if @oauth_token.respond_to?(:provided_oauth_verifier=) @oauth_token.provided_oauth_verifier=request_proxy.oauth_verifier end diff --git a/lib/carto/domain_patcher_request_proxy.rb b/lib/carto/domain_patcher_request_proxy.rb new file mode 100644 index 000000000000..1d734e6ce198 --- /dev/null +++ b/lib/carto/domain_patcher_request_proxy.rb @@ -0,0 +1,11 @@ +require 'oauth' + +module Carto + class DomainPatcherRequestProxy < OAuth::RequestProxy::RackRequest + + def uri + super.sub('carto.com', 'cartodb.com') + end + + end +end diff --git a/spec/models/carto/helpers/user_migration_helper.rb b/spec/models/carto/helpers/user_migration_helper.rb index f32a07f2c7aa..06837af84d32 100644 --- a/spec/models/carto/helpers/user_migration_helper.rb +++ b/spec/models/carto/helpers/user_migration_helper.rb @@ -7,7 +7,7 @@ module UserMigrationHelper { name: 'user-mover', description: 'insanity' } ] - agg_ds_config = + agg_ds_config = { aggregation_tables: { 'host' => 'localhost', @@ -62,7 +62,7 @@ module UserMigrationHelper puts export.log.entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) - @carto_user.client_applications.each(&:destroy) + @carto_user.client_application.destroy @table1.table_visualization.layers.each(&:destroy) @table1.destroy expect { @table1.records }.to raise_error @@ -180,7 +180,8 @@ def import end def destroy_user - @carto_user.client_applications.each(&:destroy) + @carto_user.client_application&.destroy @user.destroy end -end \ No newline at end of file + +end diff --git a/spec/models/carto/user_migration_api_key_spec.rb b/spec/models/carto/user_migration_api_key_spec.rb index 4fa747eee0f2..6fe9eeb5a301 100644 --- a/spec/models/carto/user_migration_api_key_spec.rb +++ b/spec/models/carto/user_migration_api_key_spec.rb @@ -404,7 +404,7 @@ class DummyTester $users_metadata.hmget("api_keys:#{username}:#{@master_api_key.token}", 'user')[0].should eq username $users_metadata.hmget("api_keys:#{username}:#{@regular_api_key.token}", 'user')[0].should eq username - @carto_user.client_applications.each(&:destroy) + @carto_user.client_application.destroy @master_api_key.destroy @table.destroy @map.destroy @@ -471,7 +471,7 @@ class DummyTester puts export.log.entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) - @carto_user.client_applications.each(&:destroy) + @carto_user.client_application.destroy @master_api_key.destroy @table.destroy @map.destroy @@ -552,7 +552,7 @@ class DummyTester puts export.log.entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) - @carto_user.client_applications.each(&:destroy) + @carto_user.client_application.destroy @master_api_key.destroy @table.destroy @map.destroy diff --git a/spec/models/carto/user_migration_base_spec.rb b/spec/models/carto/user_migration_base_spec.rb index 73157d2485d8..c8a9b00a072f 100644 --- a/spec/models/carto/user_migration_base_spec.rb +++ b/spec/models/carto/user_migration_base_spec.rb @@ -245,7 +245,7 @@ def teardown_mock_plpython_function(user) puts export.log.entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) - carto_user.client_applications.each(&:destroy) + carto_user.client_application.destroy user.destroy import = Carto::UserMigrationImport.create( @@ -443,4 +443,4 @@ def remove_user(carto_user) end end end -end \ No newline at end of file +end diff --git a/spec/models/user_part_crud_spec.rb b/spec/models/user_part_crud_spec.rb index 19fc6eedb7a6..1f205c59a7ec 100644 --- a/spec/models/user_part_crud_spec.rb +++ b/spec/models/user_part_crud_spec.rb @@ -220,15 +220,15 @@ it 'deletes client_application and friends' do user = create_user(email: 'clientapp@example.com', username: 'clientapp', password: @user_password) - user.create_client_application - user.client_application.access_tokens << Carto::AccessToken.new( + user.new_client_application + user.client_application.access_tokens << Carto::AccessToken.create( token: "access_token", secret: "access_secret", callback_url: "http://callback2", verifier: "v2", scope: nil, client_application_id: user.client_application.id - ).save + ) user.client_application.oauth_tokens << Carto::OauthToken.create!( token: "oauth_token", @@ -241,15 +241,17 @@ base_key = "rails:oauth_access_tokens:#{user.client_application.access_tokens.first.token}" - client_application = ClientApplication.where(user_id: user.id).first - expect(ClientApplication.where(user_id: user.id).count).to eq 2 - expect(client_application.tokens).to_not be_empty - expect(client_application.tokens.length).to eq 2 + client_application = Carto::ClientApplication.find_by(user_id: user.id) + + expect(Carto::ClientApplication.where(user_id: user.id).count).to eq 2 + tokens = Carto::OauthToken.where(client_application_id: client_application.id) + expect(tokens).to_not be_empty + expect(tokens.length).to eq 2 $api_credentials.keys.should include(base_key) user.destroy - expect(ClientApplication.where(user_id: user.id).first).to be_nil + expect(Carto::ClientApplication.find_by(user_id: user.id)).to be_nil expect(Carto::AccessToken.where(user_id: user.id).first).to be_nil expect(Carto::OauthToken.where(user_id: user.id).first).to be_nil $api_credentials.keys.should_not include(base_key) diff --git a/spec/requests/sessions_spec.rb b/spec/requests/sessions_spec.rb index 56351317ab82..1c096c52ddd6 100644 --- a/spec/requests/sessions_spec.rb +++ b/spec/requests/sessions_spec.rb @@ -60,10 +60,9 @@ end scenario "Get the session information via OAuth" do - # TODO: migrate this factory to AR (only used here) - sequel_client_application = create_client_application(user: @user, url: CartoDB.hostname, callback_url: CartoDB.hostname) - client_application = Carto::ClientApplication.find(sequel_client_application.id) - + client_application = create_client_application( + user: @user.carto_user, url: CartoDB.hostname, callback_url: CartoDB.hostname + ) oauth_consumer = OAuth::Consumer.new(client_application.key, client_application.secret, { :site => client_application.url, :scheme => :query_string, diff --git a/spec/services/carto/user_metadata_export_service_spec.rb b/spec/services/carto/user_metadata_export_service_spec.rb index 145458bd4971..bb3530ffcb53 100644 --- a/spec/services/carto/user_metadata_export_service_spec.rb +++ b/spec/services/carto/user_metadata_export_service_spec.rb @@ -72,21 +72,21 @@ def create_user_with_basemaps_assets_visualizations @user.reload # Client Application tokens - sequel_user.client_application.access_tokens << Carto::AccessToken.new( - token: "access_token", - secret: "access_secret", - callback_url: "http://callback2", - verifier: "v2", + @user.client_application.access_tokens << Carto::AccessToken.create!( + token: 'access_token', + secret: 'access_secret', + callback_url: 'http://callback2', + verifier: 'v2', scope: nil, - client_application_id: sequel_user.client_application.id - ).save - sequel_user.client_application.oauth_tokens << Carto::OauthToken.create!( - token: "oauth_token", - secret: "oauth_secret", - callback_url: "http//callback.com", - verifier: "v1", + client_application_id: @user.client_application.id + ) + @user.client_application.oauth_tokens << Carto::OauthToken.create!( + token: 'oauth_token', + secret: 'oauth_secret', + callback_url: 'http//callback.com', + verifier: 'v1', scope: nil, - client_application_id: sequel_user.client_application.id + client_application_id: @user.client_application.id ) Carto::UserMultifactorAuth.create!(user_id: @user.id, type: 'totp', last_login: Time.zone.now) @@ -172,7 +172,6 @@ def destroy_user(user = @user) st.destroy end end - ClientApplication.where(user_id: @user.id).each(&:destroy) @user.oauth_app_users.each do |oau| unless oau.oauth_access_tokens.blank? @@ -198,7 +197,7 @@ def test_import_user_from_export(export) end it 'imports latest' do - user = test_import_user_from_export(full_export) + test_import_user_from_export(full_export) end it 'imports 1.0.16 (without email_verification)' do @@ -274,7 +273,7 @@ def test_import_user_from_export(export) it 'imports 1.0.5 (without client_application)' do user = test_import_user_from_export(full_export_one_zero_five) - expect(user.client_applications).to be_empty + expect(user.client_application).to be_nil end it 'imports 1.0.4 (without synchornization oauths nor connector configurations)' do diff --git a/spec/support/factories/client_applications.rb b/spec/support/factories/client_applications.rb index a5707f31c402..f37737cb04aa 100644 --- a/spec/support/factories/client_applications.rb +++ b/spec/support/factories/client_applications.rb @@ -3,12 +3,13 @@ module Factories def new_client_application(attributes = {}) attributes = attributes.dup attributes[:user] ||= create_user - ClientApplication.new(attributes) + Carto::ClientApplication.new(attributes) end def create_client_application(attributes = {}) client_application = new_client_application(attributes) client_application.save + client_application end end -end \ No newline at end of file +end