gnikyt / laravel-shopify

A full-featured Laravel package for aiding in Shopify App development
MIT License
1.24k stars 374 forks source link

"Shopify App Outdated" but running latest app bridge, osiset, etc #1250

Open talktohenryj opened 1 year ago

talktohenryj commented 1 year ago

For bug reporting only! If you're posting a feature request or discussion, please ignore.

Expected Behavior

When going to the app it should go directly to the home page.

Current Behavior

When going into the app, I receive this message.

Frame 58

Failure Information

This happens every time I try to log into my app on production. It does not show on development. When I click the link provided, it says I am outdated because I am not compatible with the new Shopify admin domain. I don't believe that is true. Below is some additional info.

Context

Please provide any relevant information about your setup. This is important in case the issue is not reproducible except for under certain conditions.

I don't have much for failure logs but I am running the latest app bridge as set by Osiset. My app is a multipage application. That info may help.

Also, after speaking with a friend he believes the issue may be: "If your app isn't a single-page application, then your server needs to persist the host value during the initial page load. On subsequent page loads, you should use the persisted host value to configure App Bridge."

If the above is the case is there a way in Osiset to set the host value as persistent? We tried to use cookies but were unsuccessful. Also, not 100% sure that is the problem.

Any advice would be helpful. This is a terrible notification from Shopify and I believe really hurting my installation rate right now.

jhaineymilevis commented 1 year ago

same problem

talktohenryj commented 1 year ago

I messaged Shopify as well so hopefully we'll get an answer soon.

nikwen commented 1 year ago

https://github.com/osiset/laravel-shopify/issues/1248 might be related.

jhaineymilevis commented 1 year ago

@talktohenryj have you fixed it?

CedricVleminckx commented 1 year ago

Same problem in a Single page application.

Shopify says our host parameter is configured wrong in app bridge.

We also opened multiple support tickets to request a dev store on the new admin.shopify.com domain, but they say they can not help us. So there is no way for us to test this :(

This was the latest response from Shopify: when we tried to open that last request to your /authenticate/token path in a new browser tab, it takes us to the legacy/store domain and They are getting a 500 error from your app if they don’t remove that host value attached to the target parameter, which is for redirection after authenticating.

tinhphantrong0612 commented 1 year ago

@CedricVleminckx Have you tried to use SHOPIFY_FRONTEND_ENGINE=REACT? You can use createApp from Shopify app bridge with host parameter and apiKey only. With SHOPIFY_FRONTEND_ENGINE=REACT, as explained in #1173, it will bypass verify.shopify middleware to the homepage without any token. Then use app bridge in the homepage to generate a token and use it, instead of redirecting to authenticate/token and from there load app bridge library from cdn and generate token from host and apiKey, then with that token, redirect to homepage with location.href. And also, a middleware called IframeProtection was added in release 17.3.1, in PR #1178, it will attach Content-Security-Policy: frame-ancestors https://{$domain} https://admin.shopify.com header to web group response. So it's not necessary to attach it yourself.

cvikenzi commented 1 year ago

Hi @CedricVleminckx and @tinhphantrong0612, i think the best solution is to remove "shopOrigin" param from app brigde contructor (as @tinhphantrong0612 wrote before). Then, i add "host" param to all app route redirects and i add "host" to billing api return_url. I have working solution without "shopOrigin" on my side now, but i do not know how to test it. I am not skilled in git, so i am sending you print screens with my changes. Hope my solution help you a bit. Archive.zip

jhaineymilevis commented 1 year ago

@cvikenzi you saved my life!!!!! the changes on VerifyShopify.php was the solutions, i was passing host param but it was empty on the first load. Adding your changes worked!!! thanks!!!!!!

@osiset you should check @cvikenzi solution for edit code pleasse

cvikenzi commented 1 year ago

@jhaineymilevis you are welcome :)

jhaineymilevis commented 1 year ago

i have madre a pull request with fixes

1260

Kyon147 commented 1 year ago

I also have a fix in the works for the billing controller at the moment https://github.com/osiset/laravel-shopify/pull/1244

I'll take a look at the #1260 but it does not look like the tests have been updated to see if the host param exists @jhaineymilevis

bugfixman commented 1 year ago

You can do it.

Add this string to VerifyShopify middleware to the handle method.

if (!$request->session()->has('host')) {
   $request->session()->put('host', $request->host);
}

It needs to replace this string host: "{{ \Request::get('host') }}" to host: "{{ \session('host') }}" in osiset default layout.

The host param will be available anywhere. And the app bridge won't be crashed.

bugfixman commented 1 year ago

I am created a pull request with my changes are described above #1261

jhaineymilevis commented 1 year ago

@bugfixman i have tested your aproach and it works, but, problem with multi store opened in the same browser, the session host dont override when i change to another store

Kyon147 commented 1 year ago

1261 has been closed because it is a duplicate and from @jhaineymilevis does not provide a solution.

Please add any suggestions to #1260

jhaineymilevis commented 1 year ago

@cvikenzi problems with installation now :( after this changes , it redirect for https://dropify.dropi.co/login and ask for shopDomain

cvikenzi commented 1 year ago

Hello @jhaineymilevis, can you please send screen recording here, where you simulate your problem? Thanks.

jhaineymilevis commented 1 year ago

@cvikenzi forgot it, was my error, thanks

cvikenzi commented 1 year ago

@jhaineymilevis, so all ok now on your side?

Kyon147 commented 1 year ago

@osiset this looks like the issue we were chatting about were Shopify for non-spa is getting harder and harder to work with.

Would be interested to see your thoughts on a path moving forward.

gnikyt commented 1 year ago

Given recent issues surrounding this (at work), my decision is to kill off Blade and default to React with continued support of the frotjend flag so people can implement (or merge in) Vue/other framework supports.

I can't see it lasting without SPA sadly anymore - there's no true freedom to develop.

On Thu, Nov 10, 2022, 11:31 Luke Walsh @.***> wrote:

@osiset https://github.com/osiset this looks like the issue we were chatting about were Shopify for non-spa is getting harder and harder to work with.

Would be interested to see your thoughts on a path moving forward.

— Reply to this email directly, view it on GitHub https://github.com/osiset/laravel-shopify/issues/1250#issuecomment-1310424766, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASO4OR7C6LD6NW2IRE3OBDWHUE6HANCNFSM6AAAAAARTQZKQM . You are receiving this because you were mentioned.Message ID: @.***>

yasir-naseer commented 1 year ago

is there any solution which can work for non SPA app as well?

Kyon147 commented 1 year ago

There's no straightforward answer @yasir-naseer as there are issues with non-spa and approval as well. Shopify don't allow any app to make requests after OAUTH other than to the homepage of the app.

So currently blade templates need to auth.token route that happens between requests to get the JWT token to work. This is currently being rejected for new apps now - so the push by Shopify to be SPA is going to be an unwritten requirement I think.

Add that to the new admin route and AppBridge, they are slowly pushing out "traditional websites/apps" where each page is a new request.

WajahatAnwar commented 1 year ago

@Kyon147 Can you update the package with @cvikenzi code? This problem can be resolved permanently?

WajahatAnwar commented 1 year ago
Screenshot 2022-11-14 at 8 42 20 PM

@cvikenzi Can you please check after implementing your instruction am getting this error.

Kyon147 commented 1 year ago

@Kyon147 Can you update the package with @cvikenzi code? This problem can be resolved permanently?

@WajahatAnwar the issue is not as simple as merging a PR, as there are issues around using blade templates now as Shopify pushes towards SPA apps.

We need to see what the best next step is for the package overall with the blade templates.

developertester786 commented 1 year ago

Hi @CedricVleminckx and @tinhphantrong0612, i think the best solution is to remove "shopOrigin" param from app brigde contructor (as @tinhphantrong0612 wrote before). Then, i add "host" param to all app route redirects and i add "host" to billing api return_url. I have working solution without "shopOrigin" on my side now, but i do not know how to test it. I am not skilled in git, so i am sending you print screens with my changes. Hope my solution help you a bit. Archive.zip

@cvikenzi You saved me man.. You are genius. Many Many Thanks!

lramhudda commented 1 year ago

Hi @CedricVleminckx and @tinhphantrong0612, i think the best solution is to remove "shopOrigin" param from app brigde contructor (as @tinhphantrong0612 wrote before). Then, i add "host" param to all app route redirects and i add "host" to billing api return_url. I have working solution without "shopOrigin" on my side now, but i do not know how to test it. I am not skilled in git, so i am sending you print screens with my changes. Hope my solution help you a bit. Archive.zip

It is working fine

Very very thank you Really nice work We are very happy God Bless You

lramhudda commented 1 year ago
Screenshot 2022-11-14 at 8 42 20 PM

@cvikenzi Can you please check after implementing your instruction am getting this error.

Please run this command - php artisan optimize

Your error will fix.............