From 02d7e1d0c40ba867bfe7a010646c5277bfdb4ad8 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 1 Jul 2021 12:35:24 +0200 Subject: [PATCH 1/4] Do not leak all pages for guest users in API controller CanCanCan does not respect any scope set before `accessible_by`. We need to make sure the additional scopes get called afterwards. --- app/controllers/alchemy/api/pages_controller.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/controllers/alchemy/api/pages_controller.rb b/app/controllers/alchemy/api/pages_controller.rb index eea42991bf..a133cfe7b0 100644 --- a/app/controllers/alchemy/api/pages_controller.rb +++ b/app/controllers/alchemy/api/pages_controller.rb @@ -7,10 +7,12 @@ class Api::PagesController < Api::BaseController # Returns all pages as json object # def index - @pages = Language.current&.pages.presence || Alchemy::Page.none # Fix for cancancan not able to merge multiple AR scopes for logged in users if cannot? :edit_content, Alchemy::Page - @pages = @pages.accessible_by(current_ability, :index) + @pages = Alchemy::Page.accessible_by(current_ability, :index) + @pages = @pages.where(language: Language.current) + else + @pages = Language.current&.pages.presence || Alchemy::Page.none end @pages = @pages.includes(*page_includes) @pages = @pages.ransack(params[:q]).result From c814a150d4eb8783016558efc798f726cd81ccba Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Thu, 1 Jul 2021 12:50:42 +0200 Subject: [PATCH 2/4] Do not leak all elements for guest user in API CanCanCan does not respect any scope set before `accessible_by`. We need to make sure the additional scopes get called afterwards. --- .../alchemy/api/elements_controller.rb | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/app/controllers/alchemy/api/elements_controller.rb b/app/controllers/alchemy/api/elements_controller.rb index 5b8c5b2efc..46d99656d1 100644 --- a/app/controllers/alchemy/api/elements_controller.rb +++ b/app/controllers/alchemy/api/elements_controller.rb @@ -9,17 +9,19 @@ class Api::ElementsController < Api::BaseController # If you want to only load a specific type of element pass ?named=an_element_name # def index - if params[:page_id].present? - @page = Page.find(params[:page_id]) - @elements = @page.elements.not_nested + # Fix for cancancan not able to merge multiple AR scopes for logged in users + if cannot? :manage, Alchemy::Element + @elements = Alchemy::Element.accessible_by(current_ability, :index) else - @elements = Element.not_nested.joins(:page_version).merge(PageVersion.published) + @elements = Alchemy::Element.all end - # Fix for cancancan not able to merge multiple AR scopes for logged in users - if cannot? :manage, Alchemy::Element - @elements = @elements.accessible_by(current_ability, :index) + @elements = @elements.not_nested.joins(:page_version).merge(PageVersion.published) + + if params[:page_id].present? + @elements = @elements.where(alchemy_pages: { id: params[:page_id] }) end + if params[:named].present? @elements = @elements.named(params[:named]) end From eb4f36bf1409bb9ddeb0d322b0e3588c4ad4b44c Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Wed, 7 Jul 2021 21:01:25 +0200 Subject: [PATCH 3/4] Only eager load if no page_id is passed Somehow with the recent fix for cancancan accessible_by the eager loading of elements contents and essences broke with "cannot eager load polymorphic association essence". Since we soft-deprecated essences anyway and this API is mostly used by the page select in the link overlay we can live with the downside --- app/controllers/alchemy/api/elements_controller.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/controllers/alchemy/api/elements_controller.rb b/app/controllers/alchemy/api/elements_controller.rb index 46d99656d1..9fb6fe129f 100644 --- a/app/controllers/alchemy/api/elements_controller.rb +++ b/app/controllers/alchemy/api/elements_controller.rb @@ -19,13 +19,15 @@ def index @elements = @elements.not_nested.joins(:page_version).merge(PageVersion.published) if params[:page_id].present? - @elements = @elements.where(alchemy_pages: { id: params[:page_id] }) + @elements = @elements.includes(:page).where(alchemy_pages: { id: params[:page_id] }) + else + @elements = @elements.includes(*element_includes) end if params[:named].present? @elements = @elements.named(params[:named]) end - @elements = @elements.includes(*element_includes).order(:position) + @elements = @elements.order(:position) render json: @elements, adapter: :json, root: "elements" end From f8db000e78c7cbc89b45c9cc840cc34f47545682 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Wed, 7 Jul 2021 21:02:24 +0200 Subject: [PATCH 4/4] Make the link overlay spec work again It was not working since we change some UI and was self-pending since. Fixed the underlying issues and make it more robust by using capybaras wait feature. --- spec/features/admin/link_overlay_spec.rb | 32 +++++++++--------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/spec/features/admin/link_overlay_spec.rb b/spec/features/admin/link_overlay_spec.rb index 418ceda9ae..b6f96accc0 100644 --- a/spec/features/admin/link_overlay_spec.rb +++ b/spec/features/admin/link_overlay_spec.rb @@ -47,7 +47,6 @@ let!(:article) do create(:alchemy_element, name: "article", - page: page1, page_version: page1.draft_version, autogenerate_contents: true) end @@ -60,29 +59,22 @@ click_link "Link text" end - begin - within "#overlay_tab_internal_link" do - expect(page).to have_selector("#s2id_page_urlname") - select2_search(page2.name, from: "Page") - click_link "apply" - end - - within "#element_#{article.id}" do - click_button "Save" - end - - within "#flash_notices" do - expect(page).to have_content "Saved element." - end + within "#overlay_tab_internal_link" do + expect(page).to have_selector("#s2id_page_urlname") + select2_search(page2.name, from: "Page") + click_button "apply" + end - click_button_with_label "Publish page" + within "#element_#{article.id}" do + click_button "Save" + end - visit "/#{page1.urlname}" + within "#flash_notices" do + expect(page).to have_content "Saved element." + end + within_frame "alchemy_preview_window" do expect(page).to have_link("Link me", href: "/#{page2.urlname}") - rescue Capybara::ElementNotFound => e - pending e.message - raise e end end end