Merge branch 'master' into upgrade-rails

NEWS.md

@@ -11,6 +11,8 @@ Development
 - Allow the use of service account credentials on Big Query import UI ([15722](https://github.com/CartoDB/cartodb/pull/15722))
 - Added subscriptions info to the visualizations ([15723](https://github.com/CartoDB/cartodb/pull/15723))
 - Add new DO catalog ([15733](https://github.com/CartoDB/cartodb/pull/15733))
+- Fix server error at OAuth when authorize for datasets:metadata + any other datasets scope ([15738](https://github.com/CartoDB/cartodb/pull/15738))
+- api/v4/datasets returns shared dataset and access mode(read, write) [15735](https://github.com/CartoDB/cartodb/pull/15735)
 
 ### Bug fixes / enhancements
 - Fix last modified check for db connectors ([#15711](https://github.com/CartoDB/cartodb/pull/15711))
@@ -21,6 +23,7 @@ Development
 - Ignore update_timestamp function on migrations ([#15710](https://github.com/CartoDB/cartodb/pull/15710))
 - Improve query builder performance ([#15725](https://github.com/CartoDB/cartodb/pull/15725))
 - Upgrade rails to 4.2.11.3 ([#15737](https://github.com/CartoDB/cartodb/pull/15737)) 
+- Avoid duplicates on data library loading ([#15720](https://github.com/CartoDB/cartodb/pull/15720))
 
 4.38.0 (2020-06-05)
 -------------------

app/controllers/carto/api/public/datasets_controller.rb

@@ -18,10 +18,10 @@ module Carto
         setup_default_rescues
         rescue_from Carto::OauthProvider::Errors::ServerError, with: :rescue_oauth_errors
 
-        VALID_ORDER_PARAMS = %i(name type).freeze
+        VALID_ORDER_PARAMS = %i(name).freeze
-        VALID_TYPE_PARAMS = %w(table view matview).freeze
+        
-
         def index
+          @master_role = @user.api_keys.master.first.db_role
           tables = @user.in_database[select_tables_query].all
           result = enrich_tables(tables)
           total = @user.in_database[count_tables_query].first[:count]
@@ -40,14 +40,6 @@ module Carto
             VALID_ORDER_PARAMS, default_order: 'name', default_order_direction: 'asc'
           )
           @offset = (@page - 1) * @per_page
-          @types = load_type
-        end
-
-        def load_type
-          types = params[:type]&.split(',').to_a
-          raise Carto::ParamInvalidError.new(:type, VALID_TYPE_PARAMS) if (types - VALID_TYPE_PARAMS).any?
-
-          types.empty? ? VALID_TYPE_PARAMS : types
         end
 
         def check_permissions
@@ -59,30 +51,41 @@ module Carto
           table_names = tables.map { |table| table[:name] }
           visualizations = table_visualizations(table_names)
           tables.map do |table|
-            visualizations.find { |visualization| visualization[:name] == table[:name] } || table
+            viz = visualizations.find { |visualization| visualization[:name] == table[:name] }
+            extra_fields = viz || default_extra_fields
+            table.merge(extra_fields)
           end
         end
 
+        def default_extra_fields
+          {
+            cartodbfied: false,
+            shared: false,
+            privacy: nil,
+            updated_at: nil
+          }
+        end        
+
         def table_visualizations(names)
           visualizations = Carto::VisualizationQueryBuilder.new
-                                                           .with_user_id(@user.id)
+                                                           .with_owned_by_or_shared_with_user_id(@user.id)
                                                            .with_name(names)
                                                            .with_type(Carto::Visualization::TYPE_CANONICAL)
                                                            .build.all
           visualizations.map do |visualization|
             {
               name: visualization.name,
-              cartodbfied: true,
-              type: 'table',
               privacy: visualization.privacy,
-              updated_at: visualization.updated_at
+              cartodbfied: true,
+              updated_at: visualization.updated_at,
+              shared: !visualization.is_owner?(@user)
             }
           end
         end
 
         def select_tables_query
           %{
-            SELECT * FROM (#{tables_and_views_query}) AS tables_and_views
+            SELECT * FROM (#{query}) AS q
             ORDER BY #{@order} #{@direction}
             LIMIT #{@per_page}
             OFFSET #{@offset}
@@ -90,18 +93,24 @@ module Carto
         end
 
         def count_tables_query
-          "SELECT COUNT(*) FROM (#{tables_and_views_query}) AS tables_and_views"
+          %{
+            SELECT COUNT(*) FROM (#{query}) AS q
+          }.squish
         end
 
-        def tables_and_views_query
+        def query
-          @types.map { |type|
+          %{
-            %{
+            SELECT table_schema, table_name as name,   
-              SELECT #{type}name AS name, '#{type}' AS type, false AS cartodbfied, NULL AS privacy, NULL AS updated_at
+              string_agg(CASE privilege_type WHEN 'SELECT' THEN 'r' ELSE 'w' END, 
-              FROM pg_#{type}s
+                        '' order by privilege_type) as mode
-              WHERE schemaname = '#{@user.database_schema}'
+            FROM information_schema.role_table_grants
-              AND #{type}owner <> 'postgres'
+            WHERE grantee='#{@master_role}' 
-            }
+              AND table_schema not in ('cartodb', 'aggregation')  
-          }.join(' UNION ').squish
+              AND grantor!='postgres'
+              AND privilege_type in ('SELECT', 'UPDATE')
+            GROUP BY table_schema,  table_name
+          }.squish
+
         end
 
         def render_paged(result, total)

config/initializers/error_notifier.rb

@@ -16,7 +16,7 @@ Rollbar.configure do |config|
     'ActionController::RoutingError' => 'ignore',
     'Sequel::DatabaseConnectionError' => 'warning',
     'ActiveRecord::RecordInvalid' => lambda do
-      |error| info_errors.any? { |message| error.to_s.downcase.include?(message) ? 'info' : 'error' }
+      |error| info_errors.any? { |message| error.to_s.downcase.include?(message) } ? 'info' : 'error'
     end
   )
 end

lib/carto/oauth_provider/scopes/default_scope.rb

@@ -10,7 +10,9 @@ module Carto
 
         def grant_section(grants)
           section = grants.find { |i| i[:type] == @type }
-          section || { type: @type, @grant_key => [] }
+          section = section || { type: @type, @grant_key => [] }
+          section[@grant_key] ||= []
+          section
         end
 
         def add_to_api_key_grants(grants, _user = nil)

lib/resque/user_db_jobs.rb

@@ -48,7 +48,10 @@ module Resque
 
         def self.perform(user_id, visualizations_api_url)
           user = ::User.where(id: user_id).first
-          user.load_common_data(visualizations_api_url) if user.should_load_common_data?
+          return unless user.should_load_common_data?
+
+          bolt = Carto::Bolt.new("user_common_data:#{user.id}:auto_index")
+          bolt.run_locked { user.load_common_data(visualizations_api_url) }
         end
       end
     end

spec/lib/carto/oauth_provider/scopes/scopes_spec.rb

@@ -346,6 +346,7 @@ describe Carto::OauthProvider::Scopes do
       let(:full_dataset_scope) { Carto::OauthProvider::Scopes::DatasetsScope.new('datasets:rw:untitled_table') }
       let(:read_dataset_scope) { Carto::OauthProvider::Scopes::DatasetsScope.new('datasets:r:untitled_table') }
       let(:schema_scope) { Carto::OauthProvider::Scopes::SchemasScope.new('schemas:c') }
+      let(:dataset_metadata_scope) { Carto::OauthProvider::Scopes::DatasetsMetadataScope.new('datasets:metadata') }
       let(:full_table_grants) do
         [
           {
@@ -404,6 +405,22 @@ describe Carto::OauthProvider::Scopes do
         ]
       end
 
+      let(:metadata_grants) do
+        [
+          {
+            apis: [
+              'sql'
+            ],
+            type: 'apis'
+          },
+          {
+            table_metadata: [],
+            type: 'database'
+          }
+        ]
+      end
+
+
       before(:all) do
         @user = mock
         @user.stubs(:database_schema).returns('wadus')
@@ -421,6 +438,20 @@ describe Carto::OauthProvider::Scopes do
         read_dataset_scope.add_to_api_key_grants(grants, @user)
         expect(grants).to(eq(read_table_grants))
       end
+
+      it 'adds metadata permissions' do
+        grants = [{ type: 'apis', apis: [] }]
+        dataset_metadata_scope.add_to_api_key_grants(grants, @user)
+        expect(grants).to(eq(metadata_grants))
+      end
+
+      it 'does add full access permissions and metadata' do
+        grants = [{ type: 'apis', apis: [] }]
+        dataset_metadata_scope.add_to_api_key_grants(grants, @user)
+        full_dataset_scope.add_to_api_key_grants(grants, @user)
+        expect(grants[1]).to have_key(:table_metadata)
+        expect(grants[1]).to have_key(:tables)
+      end
     end
   end
 

spec/requests/carto/api/public/datasets_controller_spec.rb

@@ -1,9 +1,13 @@
 require 'spec_helper_min'
 require 'support/helpers'
+require 'factories/organizations_contexts'
 
 describe Carto::Api::Public::DatasetsController do
   include_context 'users helper'
+  include_context 'organization with users helper'
+
   include HelperMethods
+  include TableSharing
 
   describe 'index' do
     before(:each) do
@@ -22,10 +26,13 @@ describe Carto::Api::Public::DatasetsController do
         expect(response.body[:total]).to eq 3
         expect(response.body[:count]).to eq 3
         expect(response.body[:result][0][:name]).to eq 'table_a'
-        expect(response.body[:result][0][:type]).to eq 'table'
+        expect(response.body[:result][0][:mode]).to eq 'rw'
         expect(response.body[:result][0][:privacy]).to eq 'private'
         expect(response.body[:result][0][:cartodbfied]).to eq true
         expect(response.body[:result][0][:updated_at]).to_not be_nil
+        expect(response.body[:result][0][:table_schema]).to eq @user1.database_schema
+        expect(response.body[:result][0][:shared]).to eq false
+
       end
     end
 
@@ -35,11 +42,16 @@ describe Carto::Api::Public::DatasetsController do
       get_json api_v4_datasets_url(@params) do |response|
         expect(response.status).to eq(200)
         expect(response.body[:total]).to eq 4
-        expect(response.body[:result][0][:name]).to eq 'non_cartodbfied_table'
+        expected_dataset = {
-        expect(response.body[:result][0][:type]).to eq 'table'
+          name: 'non_cartodbfied_table',
-        expect(response.body[:result][0][:privacy]).to be_nil
+          mode: 'rw',
-        expect(response.body[:result][0][:cartodbfied]).to eq false
+          privacy: nil,
-        expect(response.body[:result][0][:updated_at]).to be_nil
+          cartodbfied: false,
+          updated_at: nil,
+          table_schema: @user1.database_schema,
+          shared: false
+        }
+        expect(response.body[:result][0]).to eq expected_dataset
       end
 
       @user1.in_database.execute('DROP TABLE non_cartodbfied_table')
@@ -52,29 +64,55 @@ describe Carto::Api::Public::DatasetsController do
         expect(response.status).to eq(200)
         expect(response.body[:total]).to eq 4
         expect(response.body[:result][0][:name]).to eq 'my_view'
-        expect(response.body[:result][0][:type]).to eq 'view'
+        expect(response.body[:result][0][:mode]).to eq 'rw'
         expect(response.body[:result][0][:privacy]).to be_nil
         expect(response.body[:result][0][:cartodbfied]).to eq false
         expect(response.body[:result][0][:updated_at]).to be_nil
+        expect(response.body[:result][0][:table_schema]).to eq @user1.database_schema
+        expect(response.body[:result][0][:shared]).to eq false
       end
 
       @user1.in_database.execute('DROP VIEW my_view')
     end
 
-    it 'includes materialized views' do
+    context 'shared datasets' do
-      @user1.in_database.execute('CREATE MATERIALIZED VIEW my_mat_view AS SELECT 5')
 
-      get_json api_v4_datasets_url(@params) do |response|
+      before(:each) do
-        expect(response.status).to eq(200)
+        host! "#{@org_user_2.username}.localhost.lan"
-        expect(response.body[:total]).to eq 4
+        @table_name = 'shared_table'
-        expect(response.body[:result][0][:name]).to eq 'my_mat_view'
+        @params = { api_key: @org_user_2.api_key, page: 1, per_page: 10 }
-        expect(response.body[:result][0][:type]).to eq 'matview'
+        @shared_table = FactoryGirl.create(:table, user_id: @org_user_1.id, name: @table_name )
-        expect(response.body[:result][0][:privacy]).to be_nil
-        expect(response.body[:result][0][:cartodbfied]).to eq false
-        expect(response.body[:result][0][:updated_at]).to be_nil
       end
 
-      @user1.in_database.execute('DROP MATERIALIZED VIEW my_mat_view')
+      it 'includes shared datasets read only' do
+        share_table_with_user(@shared_table, @org_user_2, access: CartoDB::Permission::ACCESS_READONLY)
+
+        get_json api_v4_datasets_url(@params) do |response|
+          expect(response.status).to eq(200)
+          expect(response.body[:total]).to eq 1
+          expect(response.body[:result][0][:name]).to eq @table_name
+          expect(response.body[:result][0][:mode]).to eq 'r'
+          expect(response.body[:result][0][:privacy]).to eq 'private'
+          expect(response.body[:result][0][:cartodbfied]).to eq true
+          expect(response.body[:result][0][:table_schema]).to eq @org_user_1.database_schema
+          expect(response.body[:result][0][:shared]).to eq true
+        end
+      end
+
+      it 'includes shared datasets read and write' do
+        share_table_with_user(@shared_table, @org_user_2, access: CartoDB::Permission::ACCESS_READWRITE)
+
+        get_json api_v4_datasets_url(@params) do |response|
+          expect(response.status).to eq(200)
+          expect(response.body[:total]).to eq 1
+          expect(response.body[:result][0][:name]).to eq @table_name
+          expect(response.body[:result][0][:mode]).to eq 'rw'
+          expect(response.body[:result][0][:privacy]).to eq 'private'
+          expect(response.body[:result][0][:cartodbfied]).to eq true
+          expect(response.body[:result][0][:table_schema]).to eq @org_user_1.database_schema
+          expect(response.body[:result][0][:shared]).to eq true
+        end
+      end
     end
 
     it 'returns 200 with an empty array if the current user does not have datasets' do
@@ -152,51 +190,6 @@ describe Carto::Api::Public::DatasetsController do
       end
     end
 
-    context 'filtering by type' do
-      before(:all) do
-        @user1.in_database.execute('CREATE VIEW my_view AS SELECT 5')
-      end
-
-      after(:all) do
-        @user1.in_database.execute('DROP VIEW my_view')
-      end
-
-      it 'filters results by table type' do
-        get_json api_v4_datasets_url(@params.merge(type: 'table')) do |response|
-          expect(response.status).to eq(200)
-          expect(response.body[:total]).to eq 3
-        end
-      end
-
-      it 'filters results by view type' do
-        get_json api_v4_datasets_url(@params.merge(type: 'view')) do |response|
-          expect(response.status).to eq(200)
-          expect(response.body[:total]).to eq 1
-        end
-      end
-
-      it 'filters results by table + view type' do
-        get_json api_v4_datasets_url(@params.merge(type: 'table,view')) do |response|
-          expect(response.status).to eq(200)
-          expect(response.body[:total]).to eq 4
-        end
-      end
-
-      it 'returns all the types by default' do
-        get_json api_v4_datasets_url(@params) do |response|
-          expect(response.status).to eq(200)
-          expect(response.body[:total]).to eq 4
-        end
-      end
-
-      it 'raises an error when type is not valid' do
-        get_json api_v4_datasets_url(@params.merge(type: 'wadus')) do |response|
-          expect(response.status).to eq(400)
-          expect(response.body[:errors]).to include("Wrong 'type' parameter")
-        end
-      end
-    end
-
     context 'ordering' do
       it 'orders results by name ascending by default' do
         get_json api_v4_datasets_url(@params) do |response|
@@ -218,20 +211,6 @@ describe Carto::Api::Public::DatasetsController do
         end
       end
 
-      it 'orders results by type descending' do
-        @user1.in_database.execute('CREATE VIEW my_view AS SELECT 5')
-
-        get_json api_v4_datasets_url(@params.merge(order: 'type', order_direction: 'desc')) do |response|
-          expect(response.status).to eq(200)
-          expect(response.body[:total]).to eq 4
-          expect(response.body[:count]).to eq 4
-          expect(response.body[:result][0][:type]).to eq 'view'
-          expect(response.body[:result][1][:type]).to eq 'table'
-        end
-
-        @user1.in_database.execute('DROP VIEW my_view')
-      end
-
       it 'returns 400 if the ordering param is invalid' do
         get_json api_v4_datasets_url(@params.merge(order: 'wadus')) do |response|
           expect(response.status).to eq(400)