gnikyt / laravel-shopify

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

Verify shopify with SPA #1173

Closed enmaboya closed 2 years ago

enmaboya commented 2 years ago

If you have a SPA frontend (React, Vue, or something else), there is no point in redirecting to the "authenticate.token" route.

Redirecting and then getting the token has the following disadvantages: 1) it takes longer to load the application; 2) the token is added to url as a get parameter.

This PR adds validation: if you specified in the settings that you are using SPA, a record is created in the table "users" and this user is not deleted, then the request is skipped.

Then you just need to describe the logic in your frontend.

Example for React:

import React from 'react';
import ReactDOM from 'react-dom/client';
import '@shopify/polaris/build/esm/styles.css';
import {AppProvider, Page, Card} from '@shopify/polaris';
import { Provider } from '@shopify/app-bridge-react';
import { AppRoutes } from './AppRoutes';

const root = ReactDOM.createRoot(document.getElementById('root'));

const host = new URLSearchParams(location.search).get("host");
const apiKey = process.env.MIX_SHOPIFY_API_KEY;

const config = { apiKey, host, forceRedirect: true };

root.render(
    <Provider config={config}>
        <AppProvider i18n={{  }}>
            <Page title="Example app">
                <Card sectioned>
                    <AppRoutes />
                </Card>
            </Page>
        </AppProvider>
    </Provider>
  );
Kyon147 commented 2 years ago

Hi @enmaboya

The one issue potentially I see is you are not loading AppBridge completely is "SPA mode" is enabled because of your conditional added in default.blade.php

This would theoretically, not load the app inside the iframe - so can you provide how this works in a real example?

enmaboya commented 2 years ago

@Kyon147 the SPA (React in my case) does not need a native AppBridge.

Instead, use the npm package "@shopify/app-bridge-react", as in the example I gave, it does the same thing - toast, modal and so on.

example

Kyon147 commented 2 years ago

Apologies, totally missed the import for app bridge!

gnikyt commented 2 years ago

Hi @enmaboya So the Reacyt package is doing something to grab a token for the user in that case?

Kyon147 commented 2 years ago

This PR also makes it very React centric, when Laravel bundles with VueJS out the box. As Shopify does not have a "Vue-App-Bridge" package, is this PR maybe constricting users on what languages and frameworks they should be using?

I say this as someone who uses React and Vue but uses Vue more exclusively with Laravel.

If we merged this in, I think we would be one-sided if we did not offer wikis on both Vue and React implementations.

Only because the React/App-Bridge handles the token out the box, with the <Provider /> wrapper. Without this, you'd not get any session token if you did turn on "SPA" mode out the box.

gnikyt commented 2 years ago

@Kyon147 True... maybe spa_frontend_used should be changed to a boolean value. Then a new config entry spa_frontend_engine where someone can put in react.

This way, react support initially, and when Vue is figured out, we can check spa_frontend_engine is vue and handle there in the future.

Kyon147 commented 2 years ago

@osiset that makes sense - I think the solution would need to be a package like Shopify's react/appbridge package, not sure if one exists so someone would have to make it ha - I do a lot of vue/shopify work so might add it to the list...

enmaboya commented 2 years ago

@osiset @Kyon147 I updated the config

now called "frontend_engine", available values: 1) blade (default value); 2) vue; 3) react;

Checking via a simple switch, if react is selected, the native app bridge won't load.

When using blade or vue, everything works as before

gnikyt commented 2 years ago

I see the error, my bad, I forgot it was Enum. Let me just pull this in locally and adjust something quick and issue a patch to this PR.

gnikyt commented 2 years ago

Adjustments made. Moved it to be string based for the config, as scalars are more user friendly for someone doing config (where possible anyways), it also allows for using environment vars to pass in the frontend engine requirement via SHOPIFY_FRONTEND_ENGINE.

Enum value for the config would've been fine, but as PHPStan complained, because the Enum methods are magic (REACT(), BLADE()) etc, its hard to see the true value. Thus, I migrated it to be string based and the Util method will convert it to the proper Enum for comparison.

I am good with this, @Kyon147 what do you think?

Kyon147 commented 2 years ago

@osiset looks good to me and I agree, I think the strings are more readable tbf and just and overall simpler implementation for the same end result.

awebartisan commented 2 years ago

@enmaboya how are you getting the token and identifying the currently logged in shop on home route when its clicked the very first time, after installation?

I am trying to make it work with Inertia.js, I can get and set the token for all Inertia ( Axios ) requests but the very first request is not Inertia (Axios). Therefore no token and hence Auth::user() returning null. Any thoughts on this?

Kyon147 commented 2 years ago

Just an update on this, billing currently does not work if you want to use the SPA only setting.

There's the Attempt to read property "plan" on null because the middleware tries using "auth" which because we have not gone to the token/auth route before the middleware, no user is logged in.

 if (Util::getShopifyConfig('billing_enabled') === true) {
            /** @var $shop IShopModel */
            $shop = auth()->user(); // on SPA we don't have any user as the authenticate/token route is skipped.
            if (! $shop->plan && ! $shop->isFreemium() && ! $shop->isGrandfathered()) {
                // They're not grandfathered in, and there is no charge or charge was declined... redirect to billing
                return Redirect::route(
                    Util::getShopifyConfig('route_names.billing'),
                    array_merge($request->input(), ['shop' => $shop->getDomain()->toNative()])
                );
            }
        }

So the billing_enabled and billing on the route break. This means we might need to add a patch and add a section in the wiki that a user is going to have to handle billing themselves if they want to use an SPA.

@osiset @enmaboya

kurakin-oleksandr commented 2 years ago

For everyone who wants to implement SPA, you can use something like this for proper handling requests

kurakin-oleksandr commented 2 years ago

Also, this patch looks like a working solution (at least it works in my case):

Index: vendor/osiset/laravel-shopify/src/Http/Middleware/Billable.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vendor/osiset/laravel-shopify/src/Http/Middleware/Billable.php b/vendor/osiset/laravel-shopify/src/Http/Middleware/Billable.php
--- a/vendor/osiset/laravel-shopify/src/Http/Middleware/Billable.php
+++ b/vendor/osiset/laravel-shopify/src/Http/Middleware/Billable.php    (date 1667258546106)
@@ -5,7 +5,9 @@
 use Closure;
 use Illuminate\Http\Request;
 use Illuminate\Support\Facades\Redirect;
+use Osiset\ShopifyApp\Contracts\Queries\Shop as ShopQuery;
 use Osiset\ShopifyApp\Contracts\ShopModel as IShopModel;
+use Osiset\ShopifyApp\Objects\Values\ShopDomain;
 use Osiset\ShopifyApp\Util;

 /**
@@ -13,6 +15,23 @@
  */
 class Billable
 {
+    /**
+     * The shop querier.
+     *
+     * @var ShopQuery
+     */
+    protected $shopQuery;
+
+    /**
+     * @param ShopQuery $shopQuery The shop querier.
+     *
+     * @return void
+     */
+    public function __construct(ShopQuery $shopQuery)
+    {
+        $this->shopQuery = $shopQuery;
+    }
+
     /**
      * Checks if a shop has paid for access.
      *
@@ -25,7 +44,7 @@
     {
         if (Util::getShopifyConfig('billing_enabled') === true) {
             /** @var $shop IShopModel */
-            $shop = auth()->user();
+            $shop = $this->shopQuery->getByDomain(ShopDomain::fromNative($request->get('shop')));
             if (! $shop->plan && ! $shop->isFreemium() && ! $shop->isGrandfathered()) {
                 // They're not grandfathered in, and there is no charge or charge was declined... redirect to billing
                 return Redirect::route(
Kyon147 commented 1 year ago

Also, this patch looks like a working solution (at least it works in my case):

Index: vendor/osiset/laravel-shopify/src/Http/Middleware/Billable.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vendor/osiset/laravel-shopify/src/Http/Middleware/Billable.php b/vendor/osiset/laravel-shopify/src/Http/Middleware/Billable.php
--- a/vendor/osiset/laravel-shopify/src/Http/Middleware/Billable.php
+++ b/vendor/osiset/laravel-shopify/src/Http/Middleware/Billable.php  (date 1667258546106)
@@ -5,7 +5,9 @@
 use Closure;
 use Illuminate\Http\Request;
 use Illuminate\Support\Facades\Redirect;
+use Osiset\ShopifyApp\Contracts\Queries\Shop as ShopQuery;
 use Osiset\ShopifyApp\Contracts\ShopModel as IShopModel;
+use Osiset\ShopifyApp\Objects\Values\ShopDomain;
 use Osiset\ShopifyApp\Util;

 /**
@@ -13,6 +15,23 @@
  */
 class Billable
 {
+    /**
+     * The shop querier.
+     *
+     * @var ShopQuery
+     */
+    protected $shopQuery;
+
+    /**
+     * @param ShopQuery $shopQuery The shop querier.
+     *
+     * @return void
+     */
+    public function __construct(ShopQuery $shopQuery)
+    {
+        $this->shopQuery = $shopQuery;
+    }
+
     /**
      * Checks if a shop has paid for access.
      *
@@ -25,7 +44,7 @@
     {
         if (Util::getShopifyConfig('billing_enabled') === true) {
             /** @var $shop IShopModel */
-            $shop = auth()->user();
+            $shop = $this->shopQuery->getByDomain(ShopDomain::fromNative($request->get('shop')));
             if (! $shop->plan && ! $shop->isFreemium() && ! $shop->isGrandfathered()) {
                 // They're not grandfathered in, and there is no charge or charge was declined... redirect to billing
                 return Redirect::route(

This looks like a good middle ground solution if people want to continue to use the built-in billing route. Can you create a PR for this @kurakin-oleksandr?

kurakin-oleksandr commented 1 year ago

@Kyon147 done 🙂 https://github.com/osiset/laravel-shopify/pull/1275