-
Notifications
You must be signed in to change notification settings - Fork 684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the unified_admin_domain
configuration option for the unified admin domain.
#1890
Conversation
347affe
to
75066d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nits, nothing blocking.
UPDATE: Also, thank you! Forgot to say that before 😅 - really appreciate you taking the time to fix this.
@@ -8,7 +8,7 @@ module FrameAncestors | |||
content_security_policy do |policy| | |||
policy.frame_ancestors(-> do | |||
domain_host = current_shopify_domain || "*.#{::ShopifyApp.configuration.myshopify_domain}" | |||
"#{ShopifyAPI::Context.host_scheme}://#{domain_host} https://admin.shopify.com" | |||
"#{ShopifyAPI::Context.host_scheme}://#{domain_host} https://admin.#{::ShopifyApp.configuration.unified_admin_domain}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use the utils method here instead for consistency?
"#{ShopifyAPI::Context.host_scheme}://#{domain_host} https://admin.#{::ShopifyApp.configuration.unified_admin_domain}" | |
"#{ShopifyAPI::Context.host_scheme}://#{domain_host} #{ShopifyApp::Utils::unified_admin_domain}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, I could make it public, should I do the same with ShopifyApp::Utils.myshopify_domain
above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally missed that it wasn't public 😅 - I am slightly concerned now that having both unified_admin_path
and unified_admin_domain
is going to lead to confusion. Maybe keep it this way for now and we think up a way of combining the two methods.
@@ -8,7 +8,7 @@ module FrameAncestors | |||
content_security_policy do |policy| | |||
policy.frame_ancestors(-> do | |||
domain_host = current_shopify_domain || "*.#{::ShopifyApp.configuration.myshopify_domain}" | |||
"#{ShopifyAPI::Context.host_scheme}://#{domain_host} https://admin.shopify.com" | |||
"#{ShopifyAPI::Context.host_scheme}://#{domain_host} https://admin.#{::ShopifyApp.configuration.unified_admin_domain}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should we add a test to test/controllers/concerns/embedded_app_test.rb
to ensure that the ancestors are being set correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some tests for the content security policy header.
7d4ecbf
to
ee8941d
Compare
ee8941d
to
9b456e4
Compare
@@ -3,7 +3,7 @@ | |||
require "test_helper" | |||
require "action_view/testing/resolvers" | |||
|
|||
class EmbeddedAppTest < ActionController::TestCase | |||
class EmbeddedAppTest < ActionDispatch::IntegrationTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was needed for Rails to populate the Content-Security-Policy
header
It is also encouraged by the Rails doc when testing controllers. (https://api.rubyonrails.org/classes/ActionController/TestCase.html)
Rails discourages the use of functional tests in favor of integration tests (use ActionDispatch::IntegrationTest).
New Rails applications no longer generate functional style controller tests and they should only be used for backward compatibility. Integration style controller tests perform actual requests, whereas functional style controller tests merely simulate a request. Besides, integration tests are as fast as functional tests and provide lot of helpers such as as, parsed_body for effective testing of controller actions including even API endpoints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!!
What this PR does
Currently, the install path assumes that the installation is done on
admin.shopify.com
if not in a spin environment. This prevent installation of the app against a local shopify server or mock server.The PR provides a
unified_admin_domain
that is used when building the installation URL will allow for installs on non-spin and non-production environments.Reviewer's guide to testing
Things to focus on
Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.md
if the changes would impact usersREADME.md
, if appropriate./docs
, if necessary