diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 0485ebb6ce..511d2d74b6 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -9,7 +9,7 @@ jobs: runs-on: ubuntu-16.04 steps: - name: Checkout current repository - uses: actions/checkout@v1 + uses: actions/checkout@v2 - name: Check NEWS.md was updated run: if git diff $(git merge-base $(git rev-parse HEAD) origin/master) --name-only | grep NEWS.md > /dev/null; then exit 0; else exit 1; fi diff --git a/.github/workflows/orm-check.yml b/.github/workflows/orm-check.yml index 5ae9f75aea..b97b62eb1d 100644 --- a/.github/workflows/orm-check.yml +++ b/.github/workflows/orm-check.yml @@ -9,7 +9,7 @@ jobs: continue-on-error: true steps: - name: Checkout current repository - uses: actions/checkout@v1 + uses: actions/checkout@v2 - name: Check if the last commit modified deprecated Sequel models shell: bash diff --git a/.github/workflows/rubocop.yml b/.github/workflows/rubocop.yml index 7e7d0872db..aaa30786c3 100644 --- a/.github/workflows/rubocop.yml +++ b/.github/workflows/rubocop.yml @@ -7,7 +7,10 @@ jobs: runs-on: ubuntu-latest steps: - name: Check out code - uses: actions/checkout@v1 + uses: actions/checkout@v2 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: 2.5 - name: Rubocop linter uses: reviewdog/action-rubocop@v1 with: diff --git a/Makefile b/Makefile index b080e024bd..26ede23c32 100644 --- a/Makefile +++ b/Makefile @@ -76,6 +76,7 @@ WORKING_SPECS_1 = \ spec/lib/carto/http/client_spec.rb \ spec/lib/carto/table_utils_spec.rb \ spec/lib/carto/authentication_manager_spec.rb \ + spec/lib/carto/g_cloud_user_settings_spec.rb \ spec/helpers/uuidhelper_spec.rb \ spec/helpers/url_validator_spec.rb \ spec/helpers/logger_helper_spec.rb \ @@ -205,6 +206,7 @@ WORKING_SPECS_5 = \ spec/commands/organization_commands/delete_spec.rb \ spec/commands/organization_commands/update_spec.rb \ spec/commands/central_user_commands_spec.rb \ + spec/commands/gcloud_user_settings_commands/set_spec.rb \ spec/commands/account_type_commands/create_spec.rb \ spec/commands/account_type_commands/update_spec.rb \ spec/commands/account_type_commands/delete_spec.rb \ diff --git a/NEWS.md b/NEWS.md index 109b10c884..04717e77bc 100644 --- a/NEWS.md +++ b/NEWS.md @@ -12,17 +12,21 @@ sudo make install ### Features - Save cloud connections data to redis [#16165](https://github.com/CartoDB/cartodb/pull/16165) +- New sign up flow [16137](https://github.com/CartoDB/cartodb/pull/16137) - New connections API [15939](https://github.com/CartoDB/cartodb/pull/15939) - New endpoints to fetch users' datasets and tilesets from their BigQuery connection [16061](https://github.com/CartoDB/cartodb/pull/16061) - New BigQuery connector [16029](https://github.com/CartoDB/cartodb/pull/16029) - Add access to DO samples. Refactor samples/subscriptions UI [#15910](https://github.com/CartoDB/cartodb/pull/15910) - Integrate new map_views metric. [#15969](https://github.com/CartoDB/cartodb/pull/15969) +- Fix named maps API retries on destroy event [#16190](https://github.com/CartoDB/cartodb/pull/16190) - Add preview/visualization of maps in DO catalog [#15973](https://github.com/CartoDB/cartodb/pull/15973) - Add new user metrics to Home page [#15950](https://github.com/CartoDB/cartodb/pull/15950) - Replace CRUD user operations in Central API client by publishing messages to the Message Broker [#16035](https://github.com/CartoDB/cartodb/pull/16035) - Adds JSON-LD with the dataset information in the Data Catalog [#16138](https://github.com/CartoDB/cartodb/pull/16138) - Add search in 'new map' screen [16166](https://github.com/CartoDB/cartodb/pull/16166) - Migrate FeatureFlag & PricePlan synchronization to the Message Broker [#16098](https://github.com/CartoDB/cartodb/pull/16098) +- Sync DO Service Account info between central and on-prem and cloud instances [#16189](https://github.com/CartoDB/cartodb/pull/16189) +- Cleanup after [#16189](https://github.com/CartoDB/cartodb/pull/16189). See [#16200](https://github.com/CartoDB/cartodb/pull/16200) - Split configuration for Message Broker & Central login redirection [#16150](https://github.com/CartoDB/cartodb/pull/16150) - Remove Data Library gallery page (now redirected to Spatial Data Catalog) [#16133](https://github.com/CartoDB/cartodb/pull/16133) @@ -64,6 +68,7 @@ sudo make install - Bump version of lib/sql submodule to 0.37.1 - Public profile can be disabled via Feature Flag [#15982](https://github.com/CartoDB/cartodb/pull/15995) - Update cartodb-common to v0.5.3, which in turns udpates pubsub to 2.3.0 [#16038](https://github.com/CartoDB/cartodb/pull/16038) +- Select distinct permissions on OAuth all datasets scope [#16196](https://github.com/CartoDB/cartodb/pull/16196) - Migrate Organization CRUD to MessageBroker [#15934](https://github.com/CartoDB/cartodb/pull/15934) - Update cartodb-common, which in turns updates the MessageBroker to send a `publisher_validation_token` [#16041](https://github.com/CartoDB/cartodb/pull/16041) - Optimize dashboard loading when the number of datasets is very large [#16014](https://github.com/CartoDB/cartodb/pull/16014) @@ -73,6 +78,7 @@ sudo make install - Upgrade deck.gl version [#16072](https://github.com/CartoDB/cartodb/pull/16072) - Configure Dead Lettering & prevent flooding of map views messages [#16059](https://github.com/CartoDB/cartodb/pull/16059) - Revamp specs for Message Broker commands and remove old endpoints [#16084](https://github.com/CartoDB/cartodb/pull/16084) +- Remove temp import tables when the IncompatibleSchemas exception is raised [#16181](https://github.com/CartoDB/cartodb/pull/16181) - Prevent rspec from being executed in any env other than test [#16128](https://github.com/CartoDB/cartodb/pull/16128) - Prevent a possible `PG::UndefinedFunction` error on DB creation [#16174](https://github.com/CartoDB/cartodb/pull/16174) - Add groups to v4/me endpoint [#16105](https://github.com/CartoDB/cartodb/pull/16105) @@ -83,6 +89,7 @@ sudo make install - Fixed error that added multiple canonical links in the Spatial Data Catalog [#16160](https://github.com/CartoDB/cartodb/pull/16160) - Modify deprecation warning for DO analysis in builder [#16163](https://github.com/CartoDB/cartodb/pull/16163) - Fix autoload issues in subscriber [#16171](https://github.com/CartoDB/cartodb/pull/16171) +- Fix CI build by changing machine spec [#16192](https://github.com/CartoDB/cartodb/pull/16192) - Update cartodb-common to v1.1.1, which contains serveral logging fixes [#16182](https://github.com/CartoDB/cartodb/pull/16182) 4.44.0 (2020-11-20) diff --git a/app/commands/gcloud_user_settings_commands/set.rb b/app/commands/gcloud_user_settings_commands/set.rb new file mode 100644 index 0000000000..6bb8c348a4 --- /dev/null +++ b/app/commands/gcloud_user_settings_commands/set.rb @@ -0,0 +1,23 @@ +module GcloudUserSettingsCommands + class Set < ::CartoCommand + + private + + def run_command + Carto::GCloudUserSettings.new(username).update(gcloud_settings) + end + + def username + params[:username] + end + + def gcloud_settings + params[:gcloud_settings] + end + + def log_context + super.merge(current_user: username) + end + + end +end diff --git a/app/connectors/importer.rb b/app/connectors/importer.rb index edb853fefc..aac656e01e 100644 --- a/app/connectors/importer.rb +++ b/app/connectors/importer.rb @@ -136,6 +136,9 @@ module CartoDB persist_metadata(name, data_import_id, overwrite) log("Table '#{name}' registered") + rescue ::CartoDB::Importer2::IncompatibleSchemas => exception + drop("#{ORIGIN_SCHEMA}.#{result.table_name}") + raise exception rescue StandardError => exception if exception.message =~ /canceling statement due to statement timeout/i drop("#{ORIGIN_SCHEMA}.#{result.table_name}") diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7068ac15a4..6441869ec6 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -199,7 +199,6 @@ class ApplicationController < ActionController::Base def check_user_state return if IGNORE_PATHS_FOR_CHECK_USER_STATE.any? { |path| request.path.end_with?("/" + path) } - viewed_username = CartoDB.extract_subdomain(request) if current_user.nil? || current_user.username != viewed_username user = Carto::User.find_by_username(viewed_username) @@ -207,6 +206,13 @@ class ApplicationController < ActionController::Base render_locked_owner return end + if user.try(:pending_verification?) + render_unverified_user + return + end + elsif current_user.try(:pending_verification?) + render_unverified_user + return elsif current_user.locked? render_locked_user return diff --git a/app/models/carto/helpers/user_commons.rb b/app/models/carto/helpers/user_commons.rb index 6cfe75185e..c6d524b2c5 100644 --- a/app/models/carto/helpers/user_commons.rb +++ b/app/models/carto/helpers/user_commons.rb @@ -305,12 +305,12 @@ module Carto::UserCommons def update_gcloud_settings(attributes) return if attributes.nil? - settings = Carto::GCloudUserSettings.new(self) + settings = Carto::GCloudUserSettings.new(self.username) settings.update attributes end def gcloud_settings - @gcloud_settings ||= Carto::GCloudUserSettings.new(self).read&.with_indifferent_access + @gcloud_settings ||= Carto::GCloudUserSettings.new(self.username).read&.with_indifferent_access end def do_enabled? diff --git a/app/models/carto/user_db_service.rb b/app/models/carto/user_db_service.rb index 5a605d8c27..cd559088ec 100644 --- a/app/models/carto/user_db_service.rb +++ b/app/models/carto/user_db_service.rb @@ -165,7 +165,15 @@ module Carto def table_grants table = Arel::Table.new('information_schema.role_table_grants') - query_sql = table.project(Arel.sql(%{ + uniq_grants = table.project(Arel.sql(%{ + DISTINCT table_schema, table_name, privilege_type + })).where(Arel.sql(%{ + grantee IN ('#{all_user_roles.join("','")}') AND + table_schema NOT IN ('cartodb', 'aggregation') AND + grantor != 'postgres' AND + privilege_type IN ('SELECT', 'UPDATE') + })).as('uniq_grants') + query_sql = Arel::SelectManager.new(Arel::Table.engine).project(Arel.sql(%{ table_schema, table_name AS name, STRING_AGG( @@ -175,12 +183,8 @@ module Carto END, '' ORDER BY privilege_type ) AS mode - })).where(Arel.sql(%{ - grantee IN ('#{all_user_roles.join("','")}') AND - table_schema NOT IN ('cartodb', 'aggregation') AND - grantor != 'postgres' AND - privilege_type IN ('SELECT', 'UPDATE') - })).group(Arel.sql('table_schema, table_name')).to_sql + })).from(uniq_grants).group(Arel.sql('table_schema, table_name')).to_sql + Arel::SelectManager.new(Arel::Table.engine, Arel.sql("(#{query_sql}) AS q")) end diff --git a/lib/carto/gcloud_user_settings.rb b/lib/carto/gcloud_user_settings.rb index 5f0cbfef9c..a22099dbf2 100644 --- a/lib/carto/gcloud_user_settings.rb +++ b/lib/carto/gcloud_user_settings.rb @@ -7,8 +7,9 @@ module Carto gcp_execution_project bq_project bq_dataset gcs_bucket).freeze - def initialize(user) - @username = user.username + def initialize(username, redis = $users_metadata) + @username = username + @redis = redis end def update(attributes) @@ -20,13 +21,13 @@ module Carto end def read - Hash[REDIS_KEYS.zip($users_metadata.hmget(key, *REDIS_KEYS))] + Hash[REDIS_KEYS.zip(@redis.hmget(key, *REDIS_KEYS))] end private def store(attributes) - $users_metadata.hmset(key, *values(attributes).to_a) + @redis.hmset(key, *values(attributes).to_a) end def values(attributes) @@ -34,7 +35,7 @@ module Carto end def remove - $users_metadata.del(key) + @redis.del(key) end def key diff --git a/lib/carto/named_maps/api.rb b/lib/carto/named_maps/api.rb index 5a0acc704f..3e8edf5c9b 100644 --- a/lib/carto/named_maps/api.rb +++ b/lib/carto/named_maps/api.rb @@ -98,7 +98,7 @@ module Carto response.response_body elsif (response_code_string.match?(/^5/) || response.code == 429) && retries < MAX_RETRY_ATTEMPTS sleep(RETRY_TIME_SECONDS**retries) - update(retries: retries + 1) + destroy(retries: retries + 1) else log_response(response, 'destroy') unless response.code == 404 end @@ -177,20 +177,22 @@ module Carto end def log_response(response, action) - log_error( - message: 'Error in named maps API', - current_user: user, - visualization_id: visualization.id, - action: action, - request_url: response.request.url, - status: response.code, - response_body: response.body - ) - rescue Encoding::UndefinedConversionError => e - # Hotfix for preventing https://rollbar.com/carto/CartoDB/items/41457 until we find the root cause - # https://cartoteam.slack.com/archives/CEQLWTW9Z/p1599134417001900 - # https://app.clubhouse.io/cartoteam/story/101908/fix-encoding-error-while-logging-request - Rollbar.error(e) + # Suppressed for now, we have to work on this trace, it's only + # generating noise without any relevant information. + # log_warning( + # message: 'Error in named maps API', + # current_user: user, + # visualization_id: visualization.id, + # action: action, + # request_url: response.request.url, + # status: response.code, + # response_body: response.body + # ) + # rescue Encoding::UndefinedConversionError => e + # # Hotfix for preventing https://rollbar.com/carto/CartoDB/items/41457 until we find the root cause + # # https://cartoteam.slack.com/archives/CEQLWTW9Z/p1599134417001900 + # # https://app.clubhouse.io/cartoteam/story/101908/fix-encoding-error-while-logging-request + # # Rollbar.error(e) end def log_context diff --git a/lib/carto/user_updater.rb b/lib/carto/user_updater.rb index 0d47203e56..a59379d2aa 100644 --- a/lib/carto/user_updater.rb +++ b/lib/carto/user_updater.rb @@ -14,7 +14,6 @@ module Carto @user.update_feature_flags(user_param[:feature_flags]) @user.regenerate_api_key(user_param[:api_key]) if user_param[:api_key].present? @user.update_rate_limits(user_param[:rate_limit]) - @user.update_gcloud_settings(user_param[:gcloud_settings]) @user.update_do_subscription(user_param[:do_subscription]) @user.save! end diff --git a/lib/tasks/central_updates_subscriber.rake b/lib/tasks/central_updates_subscriber.rake index 4d69f50fb3..2befc906ba 100644 --- a/lib/tasks/central_updates_subscriber.rake +++ b/lib/tasks/central_updates_subscriber.rake @@ -121,6 +121,13 @@ namespace :message_broker do ).run end + subscription.register_callback(:set_do_gcloud_settings) do |message| + GcloudUserSettingsCommands::Set.new( + message.payload, + { logger: logger, request_id: message.request_id } + ).run + end + at_exit do logger.info(message: 'Stopping subscriber...') subscription.stop! diff --git a/private b/private index 4227b8245b..0d530a06c2 160000 --- a/private +++ b/private @@ -1 +1 @@ -Subproject commit 4227b8245b48062e4e2598860294a40a88dde9b6 +Subproject commit 0d530a06c2998c9ecaaae2236639c4f7d654de27 diff --git a/script/ci/cloudbuild-build-pr-branch.yaml b/script/ci/cloudbuild-build-pr-branch.yaml index dee6d2c77b..8451ef6273 100644 --- a/script/ci/cloudbuild-build-pr-branch.yaml +++ b/script/ci/cloudbuild-build-pr-branch.yaml @@ -100,4 +100,6 @@ steps: substitutions: _BRANCH_TAG: ${BRANCH_NAME//\//-} +options: + machineType: 'E2_HIGHCPU_8' timeout: 1800s diff --git a/spec/commands/central_user_commands_spec.rb b/spec/commands/central_user_commands_spec.rb index f151958228..2c9fc80580 100644 --- a/spec/commands/central_user_commands_spec.rb +++ b/spec/commands/central_user_commands_spec.rb @@ -206,77 +206,6 @@ describe CentralUserCommands do ) end end - - context 'when handling Google Cloud settings' do - let(:google_cloud_settings) do - { - service_account: { - type: 'service_account', - project_id: 'my_project_id', - private_key_id: 'my_private_key_id' - }.to_json, - bq_public_project: 'my_public_project', - gcp_execution_project: 'my_gcp_execution_project', - bq_project: 'my_bq_project', - gcs_bucket: 'my_gcs_bucket', - bq_dataset: 'my_bq_dataset' - } - end - let(:user_params) do - { remote_user_id: original_user.id, gcloud_settings: google_cloud_settings } - end - let(:redis_settings) { $users_metadata.hgetall(user_settings_key).symbolize_keys } - let(:user_settings_key) { "do_settings:#{original_user.username}" } - - context 'when receiving settings values' do - it 'updates the setting values in Redis' do - central_user_commands.update_user(message) - - expect(redis_settings[:service_account]).to eq(google_cloud_settings[:service_account]) - expect(redis_settings[:bq_public_project]).to eq(google_cloud_settings[:bq_public_project]) - expect(redis_settings[:gcp_execution_project]).to eq(google_cloud_settings[:gcp_execution_project]) - expect(redis_settings[:bq_project]).to eq(google_cloud_settings[:bq_project]) - expect(redis_settings[:gcs_bucket]).to eq(google_cloud_settings[:gcs_bucket]) - expect(redis_settings[:bq_dataset]).to eq(google_cloud_settings[:bq_dataset]) - end - end - - context 'when receiving an empty hash' do - let(:google_cloud_settings) { {} } - - it 'receiving an empty hash clears the settings' do - central_user_commands.update_user(message) - - expect(redis_settings).to eq({}) - end - end - - context 'when Google Cloud settings are missing' do - let(:user_params) { { remote_user_id: original_user.id, quota_in_bytes: 1 } } - - it 'does not alter current settings' do - central_user_commands.update_user( - Carto::Common::MessageBroker::Message.new( - payload: user_params.merge(gcloud_settings: google_cloud_settings) - ) - ) - - expect(redis_settings[:service_account]).to be_present - - central_user_commands.update_user( - Carto::Common::MessageBroker::Message.new(payload: user_params) - ) - - expect($users_metadata.hgetall(user_settings_key)['service_account']).to be_present - end - - it 'an empty key is not added to Redis' do - central_user_commands.update_user(message) - - expect(redis_settings).to eq({}) - end - end - end end describe '#create_user' do diff --git a/spec/commands/gcloud_user_settings_commands/set_spec.rb b/spec/commands/gcloud_user_settings_commands/set_spec.rb new file mode 100644 index 0000000000..6050711eee --- /dev/null +++ b/spec/commands/gcloud_user_settings_commands/set_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' + +describe GcloudUserSettingsCommands::Set do + let(:command) { described_class.new(params) } + let(:sample_attributes) do + { + service_account: 'service_account_joe', + bq_public_project: 'bq_public_project_joe', + gcp_execution_project: 'gcp_execution_project_joe', + bq_project: 'bq_project_joe', + bq_dataset: 'bq_dataset_joe', + gcs_bucket: 'gcs_bucket_joe' + }.with_indifferent_access + end + + describe '#run' do + before { command.run } + + after { $users_metadata.del('do_settings:joe') } + + context 'when receiving some settings and not having anything in redis' do + let(:params) do + { + username: 'joe', + gcloud_settings: sample_attributes + } + end + + it 'updates the settings in redis' do + expect($users_metadata.hgetall('do_settings:joe')).to eq sample_attributes + end + end + + context 'when settings are nil but having some prior in redis' do + before do + settings.update(sample_attributes) + command.run + end + + after { $users_metadata.del('do_settings:joe') } + + let(:settings) { Carto::GCloudUserSettings.new('joe') } + let(:params) do + { + username: 'joe', + gcloud_settings: nil + } + end + + it 'updates the settings in redis' do + expect($users_metadata.hgetall('do_settings:joe')).to be_empty + end + end + end +end diff --git a/spec/lib/carto/g_cloud_user_settings_spec.rb b/spec/lib/carto/g_cloud_user_settings_spec.rb new file mode 100644 index 0000000000..bf518c0766 --- /dev/null +++ b/spec/lib/carto/g_cloud_user_settings_spec.rb @@ -0,0 +1,77 @@ +require 'active_support' +require 'active_support/core_ext/object/blank' +require 'active_support/core_ext/hash/keys' +require 'active_support/core_ext/hash/indifferent_access' +require_relative '../../../lib/carto/gcloud_user_settings' +require 'mock_redis' + +describe Carto::GCloudUserSettings do + let(:redis) { MockRedis.new } + let(:settings) { described_class.new('pepito', redis) } + let(:sample_attributes) do + { + service_account: 'service_account_pepito', + bq_public_project: 'bq_public_project_pepito', + gcp_execution_project: 'gcp_execution_project_pepito', + bq_project: 'bq_project_pepito', + bq_dataset: 'bq_dataset_pepito', + gcs_bucket: 'gcs_bucket_pepito' + }.with_indifferent_access + end + + describe '#initialize' do + it 'takes a username as input' do + expect { described_class.new('juanito') }.not_to raise_error + end + + it 'lets you inject a redis instance' do + expect { described_class.new('juanito', redis) }.not_to raise_error + end + end + + context 'with empty redis entries' do + describe '#update' do + it 'stores in redis the attributes passed' do + settings.update(sample_attributes) + expect(redis.hgetall('do_settings:pepito')).to eq sample_attributes + end + + it 'ignores attributes that are not part of REDIS_KEYS' do + settings.update( + service_account: 'service_account_pepito', + foo: 'bar' + ) + expected_values = { 'service_account' => 'service_account_pepito' } + expect(redis.hgetall('do_settings:pepito')).to eq expected_values + end + end + + describe '#read' do + it 'returns empty values from redis' do + expect(settings.read).to eq sample_attributes.symbolize_keys.map { |k, _| [k, nil] }.to_h + end + end + end + + context 'with some pre-existing redis entries' do + before { settings.update(sample_attributes) } + + describe '#update' do + it 'removes the redis hash when attributes are nil' do + settings.update(nil) + expect(redis.exists('do_settings:pepito')).to eq 0 + end + + it 'removes the redis hash when attributes are emtpy' do + settings.update({}) + expect(redis.exists('do_settings:pepito')).to eq 0 + end + end + + describe '#read' do + it 'returns the expected values from redis' do + expect(settings.read).to eq sample_attributes.symbolize_keys + end + end + end +end