From 38bec116ad08b22b922d09d20f378fbf8027ac4f Mon Sep 17 00:00:00 2001 From: juanrmn Date: Thu, 11 Jun 2020 17:27:47 +0200 Subject: [PATCH 01/14] avoid rollbar exception --- config/initializers/error_notifier.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/error_notifier.rb b/config/initializers/error_notifier.rb index 363ffbbbe0..1e9f035a85 100644 --- a/config/initializers/error_notifier.rb +++ b/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 From 17d14641f4dd3c8a72ad043f8cf9bbeadf128d6f Mon Sep 17 00:00:00 2001 From: juanrmn Date: Mon, 22 Jun 2020 14:08:33 +0200 Subject: [PATCH 02/14] using bolt to avoid duplicates --- lib/resque/user_db_jobs.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/resque/user_db_jobs.rb b/lib/resque/user_db_jobs.rb index f5a9c56731..fb0acbdcb8 100644 --- a/lib/resque/user_db_jobs.rb +++ b/lib/resque/user_db_jobs.rb @@ -48,7 +48,9 @@ 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? + bolt = Carto::Bolt.new("user_common_data:#{user.id}:auto_index") + + bolt.run_locked { user.load_common_data(visualizations_api_url) if user.should_load_common_data? } end end end From 398372c48825bc16a4aa2b071beb5296edc82985 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ram=C3=B3n?= Date: Mon, 22 Jun 2020 15:07:05 +0200 Subject: [PATCH 03/14] Update lib/resque/user_db_jobs.rb perfect Co-authored-by: Gonzalo Riestra --- lib/resque/user_db_jobs.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/resque/user_db_jobs.rb b/lib/resque/user_db_jobs.rb index fb0acbdcb8..312bb26947 100644 --- a/lib/resque/user_db_jobs.rb +++ b/lib/resque/user_db_jobs.rb @@ -48,9 +48,10 @@ module Resque def self.perform(user_id, visualizations_api_url) user = ::User.where(id: user_id).first - bolt = Carto::Bolt.new("user_common_data:#{user.id}:auto_index") + return unless user.should_load_common_data? - bolt.run_locked { user.load_common_data(visualizations_api_url) if 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 From 8eb6060f9b380bc326fe7d66e8262d89d5e236f3 Mon Sep 17 00:00:00 2001 From: Alberto Asuero Date: Tue, 7 Jul 2020 07:01:04 +0000 Subject: [PATCH 04/14] Return shared tables and mode at api/v4/datasets --- .../carto/api/public/datasets_controller.rb | 52 ++++++++++--------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/app/controllers/carto/api/public/datasets_controller.rb b/app/controllers/carto/api/public/datasets_controller.rb index 5e853db95f..95b3f5d73b 100644 --- a/app/controllers/carto/api/public/datasets_controller.rb +++ b/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_TYPE_PARAMS = %w(table view matview).freeze + VALID_ORDER_PARAMS = %i(name).freeze def index + @master_role = @user.api_keys.find_by(type: 'master').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,22 +51,23 @@ 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 + table[:shared] = true unless table[:table_schema]==@user.username + viz = visualizations.find { |visualization| visualization[:name] == table[:name] } || {} + viz.merge(table) end 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, + cartodbfied: true, updated_at: visualization.updated_at } end @@ -82,7 +75,7 @@ module Carto 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 +83,27 @@ 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 - @types.map { |type| - %{ - SELECT #{type}name AS name, '#{type}' AS type, false AS cartodbfied, NULL AS privacy, NULL AS updated_at - FROM pg_#{type}s - WHERE schemaname = '#{@user.database_schema}' - AND #{type}owner <> 'postgres' - } - }.join(' UNION ').squish + def query + %{ + WITH user_tables as ( + SELECT table_schema, table_name, + CASE privilege_type WHEN 'SELECT' THEN 'r' + ELSE 'w' + END as mode + FROM information_schema.role_table_grants + WHERE grantee='#{@master_role}' + AND table_schema not in ('public','cartodb', 'aggregation') + AND privilege_type in ('SELECT','UPDATE') + ) + SELECT table_schema, table_name as name, array_to_string(array_agg(mode),'') as mode + FROM user_tables + GROUP BY table_schema, table_name + }.squish end def render_paged(result, total) From 85e9b4b379c379a98f9551d58b99e2659c1b0cf1 Mon Sep 17 00:00:00 2001 From: Alberto Asuero Date: Tue, 7 Jul 2020 18:45:45 +0000 Subject: [PATCH 05/14] Add some tests --- NEWS.md | 1 + .../carto/api/public/datasets_controller.rb | 29 ++-- .../api/public/datasets_controller_spec.rb | 126 ++++++++---------- 3 files changed, 67 insertions(+), 89 deletions(-) diff --git a/NEWS.md b/NEWS.md index 4950c58b44..c691661a80 100644 --- a/NEWS.md +++ b/NEWS.md @@ -9,6 +9,7 @@ Development - Filter support when license DO datasets ([15705](https://github.com/CartoDB/cartodb/pull/15705])) - Synchronize REDIS when licensing from superadmin ([15719](https://github.com/CartoDB/cartodb/pull/15719])) - Allow the use of service account credentials on Big Query import UI ([15722](https://github.com/CartoDB/cartodb/pull/15722)) +- 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)) diff --git a/app/controllers/carto/api/public/datasets_controller.rb b/app/controllers/carto/api/public/datasets_controller.rb index 95b3f5d73b..3aab92e8f6 100644 --- a/app/controllers/carto/api/public/datasets_controller.rb +++ b/app/controllers/carto/api/public/datasets_controller.rb @@ -19,7 +19,7 @@ module Carto rescue_from Carto::OauthProvider::Errors::ServerError, with: :rescue_oauth_errors VALID_ORDER_PARAMS = %i(name).freeze - + def index @master_role = @user.api_keys.find_by(type: 'master').db_role tables = @user.in_database[select_tables_query].all @@ -52,7 +52,7 @@ module Carto visualizations = table_visualizations(table_names) tables.map do |table| table[:shared] = true unless table[:table_schema]==@user.username - viz = visualizations.find { |visualization| visualization[:name] == table[:name] } || {} + viz = visualizations.find { |visualization| visualization[:name] == table[:name] } || { cartodbfied: false} viz.merge(table) end end @@ -90,20 +90,17 @@ module Carto def query %{ - WITH user_tables as ( - SELECT table_schema, table_name, - CASE privilege_type WHEN 'SELECT' THEN 'r' - ELSE 'w' - END as mode - FROM information_schema.role_table_grants - WHERE grantee='#{@master_role}' - AND table_schema not in ('public','cartodb', 'aggregation') - AND privilege_type in ('SELECT','UPDATE') - ) - SELECT table_schema, table_name as name, array_to_string(array_agg(mode),'') as mode - FROM user_tables - GROUP BY table_schema, table_name - }.squish + SELECT table_schema, table_name as name, + string_agg(CASE privilege_type WHEN 'SELECT' THEN 'r' ELSE 'w' END, + '' order by privilege_type) as mode + FROM information_schema.role_table_grants + WHERE grantee='#{@master_role}' + AND table_schema not in ('cartodb', 'aggregation') + AND grantor!='postgres' + AND privilege_type in ('SELECT', 'UPDATE') + GROUP BY table_schema,  table_name + }.squish + end def render_paged(result, total) diff --git a/spec/requests/carto/api/public/datasets_controller_spec.rb b/spec/requests/carto/api/public/datasets_controller_spec.rb index 8fb0c26e4d..1dbe899eee 100644 --- a/spec/requests/carto/api/public/datasets_controller_spec.rb +++ b/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,7 +26,7 @@ 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 @@ -36,7 +40,7 @@ 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 'non_cartodbfied_table' - 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 be_nil expect(response.body[:result][0][:cartodbfied]).to eq false expect(response.body[:result][0][:updated_at]).to be_nil @@ -52,7 +56,7 @@ 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 @@ -61,22 +65,57 @@ describe Carto::Api::Public::DatasetsController do @user1.in_database.execute('DROP VIEW my_view') end - it 'includes materialized views' do - @user1.in_database.execute('CREATE MATERIALIZED VIEW my_mat_view AS SELECT 5') + context 'shared datasets' 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 'my_mat_view' - expect(response.body[:result][0][:type]).to eq 'matview' - 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 + before(:each) do + host! "#{@org_user_2.username}.localhost.lan" + @table_name = 'shared_table' + @params = { api_key: @org_user_2.api_key, page: 1, per_page: 10 } + @shared_table = FactoryGirl.create(:table, user_id: @org_user_1.id, name: @table_name ) + end + + 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 + end end - @user1.in_database.execute('DROP MATERIALIZED VIEW my_mat_view') + 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 + end + end end + # + # it 'includes materialized views' do + # @user1.in_database.execute('CREATE MATERIALIZED VIEW my_mat_view AS SELECT 5') + # 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 'my_mat_view' + # 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') + # end + it 'returns 200 with an empty array if the current user does not have datasets' do @user3 = FactoryGirl.create(:valid_user) host! "#{@user3.username}.localhost.lan" @@ -152,51 +191,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 +212,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) From 301f9b18648af5e39976d37cf2a9ed3d8c1411bd Mon Sep 17 00:00:00 2001 From: Alberto Asuero <1161870+alasarr@users.noreply.github.com> Date: Wed, 8 Jul 2020 16:25:08 +0200 Subject: [PATCH 06/14] Update app/controllers/carto/api/public/datasets_controller.rb Co-authored-by: Gonzalo Riestra --- app/controllers/carto/api/public/datasets_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/carto/api/public/datasets_controller.rb b/app/controllers/carto/api/public/datasets_controller.rb index 3aab92e8f6..69a5585796 100644 --- a/app/controllers/carto/api/public/datasets_controller.rb +++ b/app/controllers/carto/api/public/datasets_controller.rb @@ -21,7 +21,7 @@ module Carto VALID_ORDER_PARAMS = %i(name).freeze def index - @master_role = @user.api_keys.find_by(type: 'master').db_role + @master_role = @user.api_keys.master.db_role tables = @user.in_database[select_tables_query].all result = enrich_tables(tables) total = @user.in_database[count_tables_query].first[:count] From a6a538cd5830d3e5bff883272573c5ae3da198ea Mon Sep 17 00:00:00 2001 From: Alberto Asuero <1161870+alasarr@users.noreply.github.com> Date: Wed, 8 Jul 2020 16:26:21 +0200 Subject: [PATCH 07/14] Update spec/requests/carto/api/public/datasets_controller_spec.rb Co-authored-by: Gonzalo Riestra --- .../carto/api/public/datasets_controller_spec.rb | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/spec/requests/carto/api/public/datasets_controller_spec.rb b/spec/requests/carto/api/public/datasets_controller_spec.rb index 1dbe899eee..1e68b2f8b3 100644 --- a/spec/requests/carto/api/public/datasets_controller_spec.rb +++ b/spec/requests/carto/api/public/datasets_controller_spec.rb @@ -101,21 +101,6 @@ describe Carto::Api::Public::DatasetsController do end end - # - # it 'includes materialized views' do - # @user1.in_database.execute('CREATE MATERIALIZED VIEW my_mat_view AS SELECT 5') - # 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 'my_mat_view' - # 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') - # end - it 'returns 200 with an empty array if the current user does not have datasets' do @user3 = FactoryGirl.create(:valid_user) host! "#{@user3.username}.localhost.lan" From 5a750df77f0d76437e4d0a73a41d4f101e4e43eb Mon Sep 17 00:00:00 2001 From: Alberto Asuero <1161870+alasarr@users.noreply.github.com> Date: Wed, 8 Jul 2020 16:28:00 +0200 Subject: [PATCH 08/14] Update app/controllers/carto/api/public/datasets_controller.rb Co-authored-by: Gonzalo Riestra --- .../carto/api/public/datasets_controller.rb | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/app/controllers/carto/api/public/datasets_controller.rb b/app/controllers/carto/api/public/datasets_controller.rb index 69a5585796..d4119d8e18 100644 --- a/app/controllers/carto/api/public/datasets_controller.rb +++ b/app/controllers/carto/api/public/datasets_controller.rb @@ -51,12 +51,21 @@ module Carto table_names = tables.map { |table| table[:name] } visualizations = table_visualizations(table_names) tables.map do |table| - table[:shared] = true unless table[:table_schema]==@user.username - viz = visualizations.find { |visualization| visualization[:name] == table[:name] } || { cartodbfied: false} - viz.merge(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_owned_by_or_shared_with_user_id(@user.id) From d0b21aaeff259f30f0193ce0af7c173e2406b8ef Mon Sep 17 00:00:00 2001 From: Alberto Asuero Date: Wed, 8 Jul 2020 17:29:25 +0000 Subject: [PATCH 09/14] Add always shared and viz to tests --- .../carto/api/public/datasets_controller.rb | 5 ++-- .../api/public/datasets_controller_spec.rb | 24 +++++++++++++++---- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/app/controllers/carto/api/public/datasets_controller.rb b/app/controllers/carto/api/public/datasets_controller.rb index d4119d8e18..b3af4bad10 100644 --- a/app/controllers/carto/api/public/datasets_controller.rb +++ b/app/controllers/carto/api/public/datasets_controller.rb @@ -21,7 +21,7 @@ module Carto VALID_ORDER_PARAMS = %i(name).freeze def index - @master_role = @user.api_keys.master.db_role + @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] @@ -77,7 +77,8 @@ module Carto name: visualization.name, privacy: visualization.privacy, cartodbfied: true, - updated_at: visualization.updated_at + updated_at: visualization.updated_at, + shared: !visualization.is_owner?(@user) } end end diff --git a/spec/requests/carto/api/public/datasets_controller_spec.rb b/spec/requests/carto/api/public/datasets_controller_spec.rb index 1e68b2f8b3..910f584991 100644 --- a/spec/requests/carto/api/public/datasets_controller_spec.rb +++ b/spec/requests/carto/api/public/datasets_controller_spec.rb @@ -30,6 +30,9 @@ describe Carto::Api::Public::DatasetsController do 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 @@ -39,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' - 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 + expected_dataset = { + name: 'non_cartodbfied_table', + mode: 'rw', + privacy: 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') @@ -60,6 +68,8 @@ describe Carto::Api::Public::DatasetsController do 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') @@ -84,6 +94,8 @@ describe Carto::Api::Public::DatasetsController do 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 @@ -97,6 +109,8 @@ describe Carto::Api::Public::DatasetsController do 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 From 71cda3b8bc725d482173ba0a4d40704d1ca957f4 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Thu, 9 Jul 2020 10:36:19 +0200 Subject: [PATCH 10/14] news --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index ffdee19702..36d95bdadd 100644 --- a/NEWS.md +++ b/NEWS.md @@ -20,6 +20,7 @@ Development - Fix navigation bar tests - 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)) +- Avoid duplicates on data library loading ([#15720](https://github.com/CartoDB/cartodb/pull/15720)) 4.38.0 (2020-06-05) ------------------- From 380207430e49de4d3ad627c89fd246388c3a9ca3 Mon Sep 17 00:00:00 2001 From: Alberto Asuero Date: Thu, 9 Jul 2020 08:39:21 +0000 Subject: [PATCH 11/14] Fix authorize datasets:metadata + other scopes --- NEWS.md | 1 + lib/carto/oauth_provider/scopes/default_scope.rb | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index ffdee19702..a702f8fea4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -11,6 +11,7 @@ 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 ### Bug fixes / enhancements - Fix last modified check for db connectors ([#15711](https://github.com/CartoDB/cartodb/pull/15711)) diff --git a/lib/carto/oauth_provider/scopes/default_scope.rb b/lib/carto/oauth_provider/scopes/default_scope.rb index be897d32dd..e287cda54b 100644 --- a/lib/carto/oauth_provider/scopes/default_scope.rb +++ b/lib/carto/oauth_provider/scopes/default_scope.rb @@ -10,7 +10,11 @@ module Carto def grant_section(grants) section = grants.find { |i| i[:type] == @type } - section || { type: @type, @grant_key => [] } + section = section || { type: @type, @grant_key => [] } + if not section.key?(@grant_key) + section[@grant_key] = [] + end + section end def add_to_api_key_grants(grants, _user = nil) From 9bc718427f042dbb748fc8d7c34bb4d23368ae9f Mon Sep 17 00:00:00 2001 From: Alberto Asuero Date: Thu, 9 Jul 2020 08:40:23 +0000 Subject: [PATCH 12/14] Add news.md --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index a702f8fea4..e07068d558 100644 --- a/NEWS.md +++ b/NEWS.md @@ -11,7 +11,7 @@ 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 +- Fix server error at OAuth when authorize for datasets:metadata + any other datasets scope ([15738](https://github.com/CartoDB/cartodb/pull/15738)) ### Bug fixes / enhancements - Fix last modified check for db connectors ([#15711](https://github.com/CartoDB/cartodb/pull/15711)) From dbd183a68da31a73799c70f445d0c2212761cfe2 Mon Sep 17 00:00:00 2001 From: Alberto Asuero <1161870+alasarr@users.noreply.github.com> Date: Thu, 9 Jul 2020 16:21:47 +0200 Subject: [PATCH 13/14] Update lib/carto/oauth_provider/scopes/default_scope.rb Co-authored-by: Gonzalo Riestra --- lib/carto/oauth_provider/scopes/default_scope.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/carto/oauth_provider/scopes/default_scope.rb b/lib/carto/oauth_provider/scopes/default_scope.rb index e287cda54b..6ef6b963da 100644 --- a/lib/carto/oauth_provider/scopes/default_scope.rb +++ b/lib/carto/oauth_provider/scopes/default_scope.rb @@ -11,9 +11,7 @@ module Carto def grant_section(grants) section = grants.find { |i| i[:type] == @type } section = section || { type: @type, @grant_key => [] } - if not section.key?(@grant_key) - section[@grant_key] = [] - end + section[@grant_key] ||= [] section end From f6dd4395d91299a7b37fbb48e65ab9a94300b000 Mon Sep 17 00:00:00 2001 From: Alberto Asuero Date: Thu, 9 Jul 2020 18:14:16 +0000 Subject: [PATCH 14/14] Add metadata datasets --- .../oauth_provider/scopes/scopes_spec.rb | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/spec/lib/carto/oauth_provider/scopes/scopes_spec.rb b/spec/lib/carto/oauth_provider/scopes/scopes_spec.rb index 74bcf06a7c..15547fe5e8 100644 --- a/spec/lib/carto/oauth_provider/scopes/scopes_spec.rb +++ b/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