Skip to content
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

Make EnsureAuthenticatedLinks compatible with AppBridge 2 #1277

Merged

Conversation

kirillplatonov
Copy link
Contributor

What this PR does

EnsureAuthenticatedLinks is currently broken and doesn't pass host header for AppBridge 2. As a result, all deep links in the app are broken.
CleanShot 2021-06-05 at 20 21 22@2x

I added the missing "host" header to redirect to the splash page to fix it.

Checklist

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users
  • Update README.md, if appropriate.
  • Update any relevant pages in /docs, if necessary
  • For security fixes, the Disclosure Policy must be followed.

@rs-ehyde
Copy link

We are trying to get our application listed and we were just told that AppBridge 2.0 is a requirement. Pretty wild that this is still broken.

@kirillplatonov
Copy link
Contributor Author

@NabeelAhsen @rezaansyed @andyw8
Hey. App Bridge 2.0 is a hard requirement for new app submissions. Any chance this bugfix can be merged?

@artyrcheek
Copy link

We are trying to get our application listed and we were just told that AppBridge 2.0 is a requirement. Pretty wild that this is still broken.

Crazy this still isnt fixed

@sshaw
Copy link
Contributor

sshaw commented Jan 6, 2022

We are trying to get our application listed and we were just told that AppBridge 2.0 is a requirement. Pretty wild that this is still broken.

This is why I avoid embedded apps. Too many headaches!

@kirillplatonov
Copy link
Contributor Author

@BranLiang I saw you merged another PR in this repo recently. Could you please review this one too?

Copy link
Contributor

@NabeelAhsen NabeelAhsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey it's been a while since I actively contributed to this gem. It seems that this PR keeps slipping through the cracks, I can help ship + release this change.

I'm approving this to unblock - but let's get this PR up to speed with the main branch and add some more test coverage in the event where host isn't specified as a param by Shopify.

@kirillplatonov kirillplatonov force-pushed the authenticated-links-app-bridge-2 branch from 5e75b50 to 1b750a7 Compare January 6, 2022 23:48
@kirillplatonov kirillplatonov force-pushed the authenticated-links-app-bridge-2 branch from 1b750a7 to 0b43c34 Compare January 6, 2022 23:49
@kirillplatonov
Copy link
Contributor Author

@NabeelAhsen Hey and Happy new year!
Thanks for taking the time and looking at this 🙏. I rebased PR from master and simplified changes a bit.

We have a few more PR's that needs some attention to fully support AppBridge 2, improve JWT integration, and make the engine compatible with Ruby 3 and Rails 7. We're using this fork for these changes but it would be awesome to move these changes to the official repo. If you don't mind I will update them and mention you so we could merge & ship the new release.

@NabeelAhsen
Copy link
Contributor

@kirillplatonov are the changes in this PR the only critical changes you've found to get this gem AppBridge 2.0 compatible for multi-pages apps?

If not, could you link to the other PRs here as well? I'd like to prioritize shipping them and getting them released as the next minor version.

@NabeelAhsen NabeelAhsen merged commit 03fa1b4 into Shopify:master Jan 7, 2022
@NabeelAhsen NabeelAhsen mentioned this pull request Jan 7, 2022
4 tasks
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems January 7, 2022 20:27 Inactive
@mjesar
Copy link

mjesar commented Jan 12, 2022

Hi
I have upgraded shopify_app gem to (18.0.3) because getting this error [ShopifyApp::EnsureAuthenticatedLinks] Redirecting to login: [ShopifyApp::LoginProtection::ShopifyDomainNotFound] Could not determine current shop domain

but after upgrading shopify_app to (18.0.3), I am still getting the same error on activate charge and frame reload in the browser
error charge

Any help regarding this will be appreciated

@kirillplatonov
Copy link
Contributor Author

kirillplatonov commented Jan 12, 2022

@mjesar you need to pass host and shop params to return_url when creating a subscription. Example:

def url_with_params(url, params)
  uri = URI(url)
  uri.query = params.compact.to_query
  uri.to_s
end

callback_url = url_with_params(subscription_callback_url, {
  shop: current_shop.shopify_domain,
  host: current_shop.host
})

recurring_application_charge = ShopifyAPI::RecurringApplicationCharge.create(
  name: "Plan Name",
  price: 100,
  return_url: callback_url,
)
fullpage_redirect_to recurring_application_charge.confirmation_url

@mjesar
Copy link

mjesar commented Jan 13, 2022

@mjesar you need to pass host and shop params to return_url when creating a subscription. Example:

def url_with_params(url, params)
  uri = URI(url)
  uri.query = params.compact.to_query
  uri.to_s
end

callback_url = url_with_params(subscription_callback_url, {
  shop: current_shop.shopify_domain,
  host: current_shop.host
})

recurring_application_charge = ShopifyAPI::RecurringApplicationCharge.create(
  name: "Plan Name",
  price: 100,
  return_url: callback_url,
)
fullpage_redirect_to recurring_application_charge.confirmation_url

Thank you! it works now
But fullpage_redirect_to didnt work for me so i used
render js: "window.top.location = '#{charge.confirmation_url}';"
this was in response to an ajax call

@kirillplatonov kirillplatonov deleted the authenticated-links-app-bridge-2 branch October 11, 2023 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants