Merge branch 'master' into feature/ch132964/sync-api-keys-both-in-on-prem-and-associated-fix-specs-3

.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

.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

.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:

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 \

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)

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

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}")

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

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?

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
 

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

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

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

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!

private

@@ -1 +1 @@
-Subproject commit 4227b8245b48062e4e2598860294a40a88dde9b6
+Subproject commit 0d530a06c2998c9ecaaae2236639c4f7d654de27

script/ci/cloudbuild-build-pr-branch.yaml

@@ -100,4 +100,6 @@ steps:
 
 substitutions: 
     _BRANCH_TAG: ${BRANCH_NAME//\//-}
+options:
+    machineType: 'E2_HIGHCPU_8'
 timeout: 1800s

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

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

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