Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Verify shopify with SPA #1173

Merged
merged 18 commits into from Sep 12, 2022
Merged

Conversation

enmaboya
Copy link
Contributor

@enmaboya enmaboya commented Jul 24, 2022

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
Copy link
Collaborator

Kyon147 commented Aug 26, 2022

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
Copy link
Contributor Author

@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.

shopify_spa_app_record.mp4

@Kyon147
Copy link
Collaborator

Kyon147 commented Aug 27, 2022

Apologies, totally missed the import for app bridge!

@gnikyt
Copy link
Owner

gnikyt commented Sep 9, 2022

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

@gnikyt gnikyt self-assigned this Sep 9, 2022
@gnikyt gnikyt added the feature Enhancement to the code label Sep 9, 2022
@Kyon147
Copy link
Collaborator

Kyon147 commented Sep 9, 2022

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
Copy link
Owner

gnikyt commented Sep 9, 2022

@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
Copy link
Collaborator

Kyon147 commented Sep 10, 2022

@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
Copy link
Contributor Author

@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

Copy link
Owner

@gnikyt gnikyt left a comment

Choose a reason for hiding this comment

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

One minor comment, see if it makes sense.

src/Util.php Outdated
{
$currentFrontendEngine = self::getShopifyConfig('frontend_engine') ?? FrontendEngine::BLADE;

switch ($currentFrontendEngine) {
Copy link
Owner

Choose a reason for hiding this comment

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

@enmaboya Switch is probably not needed since its only React we need to look for.

return !$currentFrontendEngine->isSame(FrontendEngine::REACT);

So "if current engine is not React, return true and use app bridge"... React will return false and not use app bridge.

@gnikyt
Copy link
Owner

gnikyt commented Sep 12, 2022

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
Copy link
Owner

gnikyt commented Sep 12, 2022

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
Copy link
Collaborator

Kyon147 commented Sep 12, 2022

@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
Copy link
Contributor

@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
Copy link
Collaborator

Kyon147 commented Oct 23, 2022

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
Copy link

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

@kurakin-oleksandr
Copy link

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
Copy link
Collaborator

Kyon147 commented Nov 10, 2022

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
Copy link

@Kyon147 done 🙂
#1275

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Enhancement to the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants