Localhost by default - remove https:// assumption throughout codebase
#1518
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nelsonwittwer
changed the title
nelsonwittwer
commented
Sep 23, 2022
nelsonwittwer
commented
Sep 23, 2022
nelsonwittwer
changed the title
nelsonwittwer
changed the title
nelsonwittwer
changed the title
https:// assumption throughout codebase
paulomarg
reviewed
Oct 12, 2022
There was a problem hiding this comment.
I think a few of these could stay as https (anything that points to a Shopify address and not host_name).
It'll still work because I'm pretty sure Shopify will go http => https, but we can reduce the number of redirects by going straight to https when hitting Shopify.
andyw8
reviewed
Oct 12, 2022
| @@ -18,7 +18,7 @@ | |||
|
|
|||
| <%= javascript_include_tag('shopify_app/top_level', crossorigin: 'anonymous', integrity: true) %> | |||
| </head> | |||
| <body data-api-key="<%= ShopifyApp.configuration.api_key %>" data-shop-origin="https://<%= @shop %>" data-host="<%= @host %>" data-redirect-url="<%= @url %>"> | |||
| <body data-api-key="<%= ShopifyApp.configuration.api_key %>" data-shop-origin="<%= ShopifyAPI::Context.host_scheme + "://" + @shop %>" data-host="<%= @host %>" data-redirect-url="<%= @url %>"> | |||
There was a problem hiding this comment.
Perhaps we could have a method like ShopifyAPI::Context.uri_for_shop(shop) to avoid awkward string manipulation in the view.
andyw8
reviewed
Oct 12, 2022
| @@ -13,7 +13,7 @@ | |||
| id: 'redirection-target', | |||
| data: { | |||
| target: { | |||
| myshopifyUrl: "https://#{current_shopify_domain}", | |||
| myshopifyUrl: ShopifyAPI::Context.host_scheme + "://" + current_shopify_domain, | |||
There was a problem hiding this comment.
Could we have a method like current_shopify_url?
nelsonwittwer
force-pushed
the
nelsonwittwer/localhost
branch
from
d5d1092 to
e77c569
Compare
andyw8
approved these changes
Oct 13, 2022
4 tasks
ghost
reviewed
Oct 20, 2022
| @@ -38,7 +38,7 @@ Rails.application.config.after_initialize do | |||
| api_key: ShopifyApp.configuration.api_key, | |||
| api_secret_key: ShopifyApp.configuration.secret, | |||
| api_version: ShopifyApp.configuration.api_version, | |||
| host_name: URI(ENV.fetch('HOST', '')).host || '', | |||
| host: ENV['HOST'], | |||
There was a problem hiding this comment.
Either this or Shopify/shopify-api-ruby#1017 is incorrect. host_name is still required by the setup function. I'm not sure if it should still be required or should be optional now. If it should be optional maybe there should be some error checking to ensure that host and host_name don't conflict?
4 tasks
fabriazza
pushed a commit
to fabriazza/shopify_app
that referenced
this pull request
Feb 1, 2023
…se (Shopify#1518) * use ShopifyAPI::Context.host instead of assume https:// * use host scheme correctly * use ShopifyAPI::Context.host instead of composition * restore domain_host pattern * always tls with current_shopify_domain * typo + revert to tls default with cookies * 12.0 shopify_api dependency * update 12.1.0 API * rubocop updates * docs, readme, and changelog for localhost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does
This PR will work in tandem with this PR in the ruby API. This will enable users to run
shopify_appwithlocalhostinstead of mandating a tunnel for OAuth redirects.This removes hard coded
https://and uses theShopifyAPI::Context.hostand orShopifyAPI::Context.host_schemeto build redirect links.Reviewer's guide to testing
Ensure any modified links still work as expected.
Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.mdif the changes would impact usersREADME.md, if appropriate./docs, if necessary