From 0a5903af9ea52e0278a68a8c80b0804a50249459 Mon Sep 17 00:00:00 2001 From: manmorjim Date: Thu, 4 Feb 2021 09:12:01 +0100 Subject: [PATCH 01/24] Redirect to unverified user when signed up --- app/controllers/application_controller.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 8368efc26d..7e523a1f19 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 From cf165c43fa9356fe34e3cd1954122e15af559d93 Mon Sep 17 00:00:00 2001 From: manmorjim Date: Thu, 11 Feb 2021 18:20:48 +0100 Subject: [PATCH 02/24] update NEWS.md --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 71ce94a027..68f0d5af17 100644 --- a/NEWS.md +++ b/NEWS.md @@ -11,6 +11,7 @@ sudo make install ### Features +- 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) From a36991e4b1d4f4a16a01989b51825d52c776bb7f Mon Sep 17 00:00:00 2001 From: cgonzalez Date: Tue, 23 Feb 2021 15:46:50 +0000 Subject: [PATCH 03/24] Remove temp import table on IncompatibleSchemas exception --- app/connectors/importer.rb | 3 +++ 1 file changed, 3 insertions(+) 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}") From d18ea4ce5236d053bc8eb9c33d60168e272a1cec Mon Sep 17 00:00:00 2001 From: cgonzalez Date: Tue, 23 Feb 2021 18:16:03 +0000 Subject: [PATCH 04/24] Update NEWS.md --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index b37ac42eaa..aa1e3ab4c8 100644 --- a/NEWS.md +++ b/NEWS.md @@ -68,6 +68,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) - Add groups to v4/me endpoint [#16105](https://github.com/CartoDB/cartodb/pull/16105) - Add deprecation warning for DO analysis in builder and hide option when user creation is later than deprecation notice date [#16118](https://github.com/CartoDB/cartodb/pull/16118) From 35c9788c106f7b798499de0baddcccf24e8670e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20L=C3=B3pez=20Ruiz?= Date: Tue, 2 Mar 2021 16:12:35 +0100 Subject: [PATCH 05/24] Minor improvements in privates (#16169) --- private | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/private b/private index 4227b8245b..05193fedb9 160000 --- a/private +++ b/private @@ -1 +1 @@ -Subproject commit 4227b8245b48062e4e2598860294a40a88dde9b6 +Subproject commit 05193fedb91bce3c2bcb9ab24ce86470af8fb51a From b7dabb42be6b2cbbc35445a087150ee7f123813d Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Thu, 4 Mar 2021 13:24:48 +0100 Subject: [PATCH 06/24] Fix named maps API retries + temporarily silence logging --- NEWS.md | 1 + lib/carto/named_maps/api.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index b37011a942..93dfb4f45c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -18,6 +18,7 @@ sudo make install - 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 - 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) diff --git a/lib/carto/named_maps/api.rb b/lib/carto/named_maps/api.rb index 5a0acc704f..7a78faf4f8 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 @@ -190,7 +190,7 @@ module Carto # 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) + # Rollbar.error(e) end def log_context From 83e47dd54943a5d5f35ea5affea8c2c0cd7b2b73 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 4 Mar 2021 13:25:59 +0100 Subject: [PATCH 07/24] Add machineType (as suggested by @alberhander) --- NEWS.md | 1 + script/ci/cloudbuild-build-pr-branch.yaml | 2 ++ 2 files changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index b37011a942..2eb1c42011 100644 --- a/NEWS.md +++ b/NEWS.md @@ -85,6 +85,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/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 From 8cea821a867bc1d35f201a2d8349d9f3cd1dd1c8 Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Thu, 4 Mar 2021 17:56:13 +0100 Subject: [PATCH 08/24] Suppress logs --- NEWS.md | 2 +- lib/carto/named_maps/api.rb | 30 ++++++++++++++++-------------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/NEWS.md b/NEWS.md index 3731c43a42..a81db1fda3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -18,7 +18,7 @@ sudo make install - 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 +- 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) diff --git a/lib/carto/named_maps/api.rb b/lib/carto/named_maps/api.rb index 7a78faf4f8..3e8edf5c9b 100644 --- a/lib/carto/named_maps/api.rb +++ b/lib/carto/named_maps/api.rb @@ -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 From 88da2c7ff2ee19d4aa9c16c8392203fa1eaebf31 Mon Sep 17 00:00:00 2001 From: cgonzalez Date: Thu, 4 Mar 2021 18:01:13 +0000 Subject: [PATCH 09/24] Select only uniq table grants --- app/models/carto/user_db_service.rb | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) 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 From 8fd5aa1896dd958c6d6cebb717516be6c7ea8207 Mon Sep 17 00:00:00 2001 From: cgonzalez Date: Thu, 4 Mar 2021 18:20:20 +0000 Subject: [PATCH 10/24] Update NEWS.md --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 2eb1c42011..d11acd2260 100644 --- a/NEWS.md +++ b/NEWS.md @@ -65,6 +65,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) From 358f4f2cd63609c98637eb0a7c37707f60959e57 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 4 Mar 2021 09:28:08 +0100 Subject: [PATCH 11/24] Change API: GCloudUserSettings to depend on username instead of User --- app/models/carto/helpers/user_commons.rb | 4 ++-- lib/carto/gcloud_user_settings.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) 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/lib/carto/gcloud_user_settings.rb b/lib/carto/gcloud_user_settings.rb index 5f0cbfef9c..773992d849 100644 --- a/lib/carto/gcloud_user_settings.rb +++ b/lib/carto/gcloud_user_settings.rb @@ -7,8 +7,8 @@ module Carto gcp_execution_project bq_project bq_dataset gcs_bucket).freeze - def initialize(user) - @username = user.username + def initialize(username) + @username = username end def update(attributes) From 59e833a331d289991bf1ba31ea66a5c472fe72a4 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 4 Mar 2021 11:18:39 +0100 Subject: [PATCH 12/24] Add command to process message of type :set_do_gcloud_settings --- .../gcloud_user_settings_commands/set.rb | 24 +++++++++++++++++++ lib/carto/user_updater.rb | 1 - lib/tasks/central_updates_subscriber.rake | 7 ++++++ 3 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 app/commands/gcloud_user_settings_commands/set.rb 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..b01df0da61 --- /dev/null +++ b/app/commands/gcloud_user_settings_commands/set.rb @@ -0,0 +1,24 @@ +module GCloudUserSettingsCommands + class Set < ::CartoCommand + + private + + def run_command + Carto::GCloudUserSettings.new(username).update(gcloud_settings) + logger.info(log_context.merge(message: 'gcloud user settings updated')) + 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/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 9b800058f9..bdde42edd2 100644 --- a/lib/tasks/central_updates_subscriber.rake +++ b/lib/tasks/central_updates_subscriber.rake @@ -107,6 +107,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! From 3dab10225daea7f55ff66569299aa2123e8f19b2 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 4 Mar 2021 18:58:42 +0100 Subject: [PATCH 13/24] Disable tests to be moved --- spec/commands/central_user_commands_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/commands/central_user_commands_spec.rb b/spec/commands/central_user_commands_spec.rb index f151958228..bd56f32d8c 100644 --- a/spec/commands/central_user_commands_spec.rb +++ b/spec/commands/central_user_commands_spec.rb @@ -208,6 +208,8 @@ describe CentralUserCommands do end context 'when handling Google Cloud settings' do + before { pending('test GCloudUserSettingsCommands::Set instead') } + let(:google_cloud_settings) do { service_account: { From e2cf112e821a6dc023e6f96b7f1457f730eb1d38 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 4 Mar 2021 19:25:14 +0100 Subject: [PATCH 14/24] Update NEWS.md --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index cf2d22e63c..15a3d647c7 100644 --- a/NEWS.md +++ b/NEWS.md @@ -25,6 +25,7 @@ sudo make install - 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) - 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) From b16d3795d522881e90785dd50f9aedc696abaf1e Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 5 Mar 2021 11:16:41 +0100 Subject: [PATCH 15/24] Restore old behavior and tests --- lib/carto/user_updater.rb | 6 ++++++ spec/commands/central_user_commands_spec.rb | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/carto/user_updater.rb b/lib/carto/user_updater.rb index a59379d2aa..c847b62259 100644 --- a/lib/carto/user_updater.rb +++ b/lib/carto/user_updater.rb @@ -14,6 +14,12 @@ 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]) + + # NOTE: For backwards compatibility, to be removed once + # https://github.com/CartoDB/cartodb-central/pull/3078 is + # deployed + @user.update_gcloud_settings(user_param[:gcloud_settings]) + @user.update_do_subscription(user_param[:do_subscription]) @user.save! end diff --git a/spec/commands/central_user_commands_spec.rb b/spec/commands/central_user_commands_spec.rb index bd56f32d8c..64bc76494f 100644 --- a/spec/commands/central_user_commands_spec.rb +++ b/spec/commands/central_user_commands_spec.rb @@ -207,9 +207,9 @@ describe CentralUserCommands do end end + # TODO: this tests old behavior, remove once + # https://github.com/CartoDB/cartodb-central/pull/3078 is merged context 'when handling Google Cloud settings' do - before { pending('test GCloudUserSettingsCommands::Set instead') } - let(:google_cloud_settings) do { service_account: { From 5b34f6826ccf1c8d1817c6b346821d796f9e7e3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?A=CC=81lvaro=20Manera?= Date: Fri, 5 Mar 2021 11:46:23 +0100 Subject: [PATCH 16/24] use checkout v2 --- .github/workflows/main.yml | 2 +- .github/workflows/orm-check.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 From 6076de40bd68e0f3fcb9e81173aa261eb7a2c102 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?A=CC=81lvaro=20Manera?= Date: Fri, 5 Mar 2021 11:46:43 +0100 Subject: [PATCH 17/24] setup ruby on rubocop --- .github/workflows/rubocop.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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: From df726cfdbac6951a572c7354951e4b2278f2945e Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 5 Mar 2021 12:50:36 +0100 Subject: [PATCH 18/24] Add ability to inject a redis connection --- lib/carto/gcloud_user_settings.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/carto/gcloud_user_settings.rb b/lib/carto/gcloud_user_settings.rb index 773992d849..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(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 From b818488f4e0044a58636e2c490ff607d3eaa067d Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 5 Mar 2021 16:45:14 +0100 Subject: [PATCH 19/24] Add tests (rather unit) to cover Carto::GCloudUserSettings --- Makefile | 1 + spec/lib/carto/g_cloud_user_settings_spec.rb | 77 ++++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 spec/lib/carto/g_cloud_user_settings_spec.rb diff --git a/Makefile b/Makefile index b080e024bd..76c0937b56 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 \ 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 From 86fb0e27d65fe3b92403662bb78ad27c4b90fbdc Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 5 Mar 2021 16:48:08 +0100 Subject: [PATCH 20/24] Remove unneeded log trace --- app/commands/gcloud_user_settings_commands/set.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/commands/gcloud_user_settings_commands/set.rb b/app/commands/gcloud_user_settings_commands/set.rb index b01df0da61..1246477b13 100644 --- a/app/commands/gcloud_user_settings_commands/set.rb +++ b/app/commands/gcloud_user_settings_commands/set.rb @@ -5,7 +5,6 @@ module GCloudUserSettingsCommands def run_command Carto::GCloudUserSettings.new(username).update(gcloud_settings) - logger.info(log_context.merge(message: 'gcloud user settings updated')) end def username From 309b5a2fcbb22dfc5d73fe01c33e4c457150f647 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 5 Mar 2021 17:28:41 +0100 Subject: [PATCH 21/24] Subtle case renaming of class to facilitate autoloading --- app/commands/gcloud_user_settings_commands/set.rb | 2 +- lib/tasks/central_updates_subscriber.rake | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/commands/gcloud_user_settings_commands/set.rb b/app/commands/gcloud_user_settings_commands/set.rb index 1246477b13..6bb8c348a4 100644 --- a/app/commands/gcloud_user_settings_commands/set.rb +++ b/app/commands/gcloud_user_settings_commands/set.rb @@ -1,4 +1,4 @@ -module GCloudUserSettingsCommands +module GcloudUserSettingsCommands class Set < ::CartoCommand private diff --git a/lib/tasks/central_updates_subscriber.rake b/lib/tasks/central_updates_subscriber.rake index bdde42edd2..c4a3a54142 100644 --- a/lib/tasks/central_updates_subscriber.rake +++ b/lib/tasks/central_updates_subscriber.rake @@ -108,7 +108,7 @@ namespace :message_broker do end subscription.register_callback(:set_do_gcloud_settings) do |message| - GCloudUserSettingsCommands::Set.new( + GcloudUserSettingsCommands::Set.new( message.payload, { logger: logger, request_id: message.request_id } ).run From 8f20445981166513a7eeed27e4b3fa47eeb898a2 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 5 Mar 2021 17:30:29 +0100 Subject: [PATCH 22/24] Add a spec for GcloudUserSettingsCommands::Set (end-to-end) --- Makefile | 1 + .../gcloud_user_settings_commands/set_spec.rb | 55 +++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 spec/commands/gcloud_user_settings_commands/set_spec.rb diff --git a/Makefile b/Makefile index 76c0937b56..26ede23c32 100644 --- a/Makefile +++ b/Makefile @@ -206,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/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 From f5eac8e4d167cfedf0994daf0bf020ecd012a595 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 5 Mar 2021 18:50:59 +0100 Subject: [PATCH 23/24] Remove code for behavior taken care of by GcloudUserSettingsCommands --- NEWS.md | 1 + lib/carto/user_updater.rb | 6 -- spec/commands/central_user_commands_spec.rb | 73 --------------------- 3 files changed, 1 insertion(+), 79 deletions(-) diff --git a/NEWS.md b/NEWS.md index 15a3d647c7..04717e77bc 100644 --- a/NEWS.md +++ b/NEWS.md @@ -26,6 +26,7 @@ sudo make install - 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) diff --git a/lib/carto/user_updater.rb b/lib/carto/user_updater.rb index c847b62259..a59379d2aa 100644 --- a/lib/carto/user_updater.rb +++ b/lib/carto/user_updater.rb @@ -14,12 +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]) - - # NOTE: For backwards compatibility, to be removed once - # https://github.com/CartoDB/cartodb-central/pull/3078 is - # deployed - @user.update_gcloud_settings(user_param[:gcloud_settings]) - @user.update_do_subscription(user_param[:do_subscription]) @user.save! end diff --git a/spec/commands/central_user_commands_spec.rb b/spec/commands/central_user_commands_spec.rb index 64bc76494f..2c9fc80580 100644 --- a/spec/commands/central_user_commands_spec.rb +++ b/spec/commands/central_user_commands_spec.rb @@ -206,79 +206,6 @@ describe CentralUserCommands do ) end end - - # TODO: this tests old behavior, remove once - # https://github.com/CartoDB/cartodb-central/pull/3078 is merged - 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 From bbba84bc17cbee1ce9c5adf9528a5f700f780cef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20L=C3=B3pez=20Ruiz?= Date: Mon, 8 Mar 2021 12:07:14 +0100 Subject: [PATCH 24/24] Update external private (#16197) --- private | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/private b/private index 05193fedb9..0d530a06c2 160000 --- a/private +++ b/private @@ -1 +1 @@ -Subproject commit 05193fedb91bce3c2bcb9ab24ce86470af8fb51a +Subproject commit 0d530a06c2998c9ecaaae2236639c4f7d654de27