gnikyt / laravel-shopify

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

Auth Flow Refactoring #744

Closed gnikyt closed 3 years ago

gnikyt commented 3 years ago

Dev Branch: [feature/cookieless]

Introduction

Shopify is now requiring apps to work with session tokens (JWT).

This package has changed several times due to requirement changes with either Shopify or browsers.

At this point, a restructure will be worked on as a priority for authentication flow and validation.

Off the top of my head as some planning, the following will be done and worked on:

Removal of cookies & JWT first-class

Still toying with this in my head, however, while the recent releases helps with cookie issues by handling ITP, its a messy solution and also breaks a good userflow when they see the warnings pop up to enable cookies.

Currently, I see abolishing all cookie and ITP related code and switch to JWT only.

This would involve the removal of ITP views, ITP middleware, cookie class, a good chunk of AuthShopify middleware, and more.

In place, we can promote AuthToken middleware to the forefront, and modify it to utilize Laravel's request context so place in the decoded JWT data into the request lifecycle.

This will allow for the JWT data to be available within the code and views, allowing us to then lookup the user based on the JWT data by request, instead of cookies.


A lot of work will be required to complete this, please be patient. If someone sees an issue with promoting JWT to the forefront, please let me know.

gnikyt commented 3 years ago

@osiset Are you able to submit POST forms? I get a 419 error no matter what I do for some reason. Regular forms that have always been there and that worked before switching to this branch. My first instinct was a collision with the CSRF token but the field names are different (_token vs token). Removing the Shopify session token field still results in a 419.

I'm digging into it right now, but wanted to check with you in case there's something obvious I missed πŸ€”

Hey, no I have yet to test forms.

squatto commented 3 years ago

The session token and the POSTed token are different for some reason πŸ€”

image

arvesolland commented 3 years ago

I am looking at adding a JS function for non-SPAs to fetch and update the token URL upon click/submit

I am implementing that into my app literally as we speak, in fact. It's the only way to make this remotely possible πŸ€¦πŸΌβ€β™‚οΈ

Once I get the initial version of my app submitted and approved, I'm going to take a few days and convert everything to use Inertia.js. I hate having to do hacky things just to make something work like normal.

@squatto - For a new app - do you think using something like intertia would make some of this easier?

squatto commented 3 years ago

@arvesolland I can definitely say that I think that Inertia.js will make it easier. I haven't tried it myself just yet, but as long as you can inject the current session token into Inertia's AJAX request, you should be good to go. I did a quick skim of the Inertia docs just now and I couldn't find any way to do that. However, it looks like it's using axios, so you can use interceptors to change the request headers.

I would be happy to provide the javascript snippet that you'll need to poll Shopify for the current session token. It sets the current session token to window.sessionToken and polls Shopify for the current token every few seconds (since they're only valid for 60 seconds). That way you can access the current token any time that you need it. The alternative - which is what you'll find examples of in this package - is to get the current session token right when you need it. I didn't opt for this method because it can potentially introduce a delay on the action being taken (e.g. click a button >> wait for Shopify to respond with the current token >> perform the action)

At this point, I would not only recommend that you avoid building a non-SPA app, I would INSIST that you avoid it! Build an SPA app. Period. I'm pulling my hair out right now trying to figure out how to make something work that is as simple as submitting a form, saving the response in a controller, and redirecting back to the app from the controller. I'm having to jump through hoop after hoop and it's driving me crazy 😫

I'm honestly just about to the point that I'm going to scrap trying to make my non-SPA app work and just rebuild the UI with Inertia. I'm spending far too long trying to hack things together.

diemah77 commented 3 years ago

I am looking at adding a JS function for non-SPAs to fetch and update the token URL upon click/submit

I am implementing that into my app literally as we speak, in fact. It's the only way to make this remotely possible πŸ€¦πŸΌβ€β™‚οΈ Once I get the initial version of my app submitted and approved, I'm going to take a few days and convert everything to use Inertia.js. I hate having to do hacky things just to make something work like normal.

@squatto - For a new app - do you think using something like intertia would make some of this easier?

We've implemented session token using Inertia in our apps. It works quite nicely. Can only recommend using it.

arvesolland commented 3 years ago

@arvesolland I can definitely say that I think that Inertia.js will make it easier. I haven't tried it myself just yet, but as long as you can inject the current session token into Inertia's AJAX request, you should be good to go. I did a quick skim of the Inertia docs just now and I couldn't find any way to do that. However, it looks like it's using axios, so you can use interceptors to change the request headers.

I would be happy to provide the javascript snippet that you'll need to poll Shopify for the current session token. It sets the current session token to window.sessionToken and polls Shopify for the current token every few seconds (since they're only valid for 60 seconds). That way you can access the current token any time that you need it. The alternative - which is what you'll find examples of in this package - is to get the current session token right when you need it. I didn't opt for this method because it can potentially introduce a delay on the action being taken (e.g. click a button >> wait for Shopify to respond with the current token >> perform the action)

At this point, I would not only recommend that you avoid building a non-SPA app, I would INSIST that you avoid it! Build an SPA app. Period. I'm pulling my hair out right now trying to figure out how to make something work that is as simple as submitting a form, saving the response in a controller, and redirecting back to the app from the controller. I'm having to jump through hoop after hoop and it's driving me crazy 😫

I'm honestly just about to the point that I'm going to scrap trying to make my non-SPA app work and just rebuild the UI with Inertia. I'm spending far too long trying to hack things together. @squatto Thanks so much for the detailed answer - would love to see the JS function πŸ™ŒπŸ»πŸ™ŒπŸ»

arvesolland commented 3 years ago

I am looking at adding a JS function for non-SPAs to fetch and update the token URL upon click/submit

I am implementing that into my app literally as we speak, in fact. It's the only way to make this remotely possible πŸ€¦πŸΌβ€β™‚οΈ

Once I get the initial version of my app submitted and approved, I'm going to take a few days and convert everything to use Inertia.js. I hate having to do hacky things just to make something work like normal.

@squatto - For a new app - do you think using something like intertia would make some of this easier?

We've implemented session token using Inertia in our apps. It works quite nicely. Can only recommend using it.

@diemah77 Awesome - do you have any advice or examples of what is needed to have inertia and this package working together ?

diemah77 commented 3 years ago

We had to fork Inertia in order to implement the default header config where the session token was placed globally, as it is not supported natively as of now.

Before the custom Inertia fork, we tried to send the token through an axios interceptor. However, this didn't work out. I guess Inertia just ignores any custom axios headers.

I'll share what we have done to set Inertia global header config as well as the JS bootstrap file by tomorrow.

squatto commented 3 years ago

@osiset Are you able to submit POST forms? I get a 419 error no matter what I do for some reason. Regular forms that have always been there and that worked before switching to this branch. My first instinct was a collision with the CSRF token but the field names are different (_token vs token). Removing the Shopify session token field still results in a 419. I'm digging into it right now, but wanted to check with you in case there's something obvious I missed πŸ€”

Hey, no I have yet to test forms.

@osiset I've been inspecting requests/responses and it looks like what's happening is that since every request is creating a new login/session, it is also regenerating a new CSRF token. That's why I was seeing a different value in the POST data from my form and the destination controller.

Here's an example of what I'm seeing. I hit /settings in my app, it redirected to /authenticate/token to get the token, and then redirected back to /settings. Each of the three requests regenerated the CSRF token.

image

This brings up a question: why can't we use standard cookies/sessions only within our app itself? It's a first-party cookie. Could you do everything that you're already doing to set/verify the token, and when you've verified it, could you check if the shop is already logged in and let it use the same session?

squatto commented 3 years ago

@squatto Thanks so much for the detailed answer - would love to see the JS function πŸ™ŒπŸ»πŸ™ŒπŸ»

You're welcome! You'll need to cut and splice this however you need to, and you'll also need to set your API key and shop domain. I got most of this from this shopify.dev tutorial:

const AppBridge = window['app-bridge'];
const utils = window['app-bridge-utils'];
const actions = AppBridge.actions;
const createApp = AppBridge.default;

const app = createApp({
  apiKey: 'API key',
  shopOrigin: 'shop domain',
  forceRedirect: true,
});

// request a new token every 5s
const SESSION_TOKEN_REFRESH_INTERVAL = 5000;

async function retrieveToken(app) {
  window.sessionToken = await utils.getSessionToken(app);
}

function keepRetrievingToken(app) {
  setInterval(() => retrieveToken(app), SESSION_TOKEN_REFRESH_INTERVAL);
}

document.addEventListener('DOMContentLoaded', async () => {
  await retrieveToken(app);
  keepRetrievingToken(app);
});

The end result is that you'll have window.sessionToken available to any of your scripts that need it. It will always be the latest/current session token because it auto-updates every 5 seconds (you can adjust how often it updates).

I've added a hidden input field to all of my forms named "token" that has a class "access-token". I automatically update the value of the input field, and I also set the "Authorization" header in all jQuery AJAX requests. You can do this by adding this to the retrieveToken() function:

// update the access token in forms
$('.access-token:input').val(window.sessionToken);

// update the authorization header sent with ajax requests
$.ajaxSettings.headers['Authorization'] = 'Bearer ' + window.sessionToken;
squatto commented 3 years ago

We had to fork Inertia in order to implement the default header config where the session token was placed globally, as it is not supported natively as of now.

Before the custom Inertia fork, we tried to send the token through an axios interceptor. However, this didn't work out. I guess Inertia just ignores any custom axios headers.

I'll share what we have done to set Inertia global header config as well as the JS bootstrap file by tomorrow.

@diemah77 Thank you very much for the information! I really appreciate it. That's good to know about the axios interceptor...well, bad that it doesn't work, but good to know, at least. Have you reached out to them about needing to change the request headers?

arvesolland commented 3 years ago

@squatto Thanks so much for the detailed answer - would love to see the JS function πŸ™ŒπŸ»πŸ™ŒπŸ»

You're welcome! You'll need to cut and splice this however you need to, and you'll also need to set your API key and shop domain. I got most of this from this shopify.dev tutorial:

const AppBridge = window['app-bridge'];
const utils = window['app-bridge-utils'];
const actions = AppBridge.actions;
const createApp = AppBridge.default;

const app = createApp({
  apiKey: 'API key',
  shopOrigin: 'shop domain',
  forceRedirect: true,
});

// request a new token every 5s
const SESSION_TOKEN_REFRESH_INTERVAL = 5000;

async function retrieveToken(app) {
  window.sessionToken = await utils.getSessionToken(app);
}

function keepRetrievingToken(app) {
  setInterval(() => retrieveToken(app), SESSION_TOKEN_REFRESH_INTERVAL);
}

document.addEventListener('DOMContentLoaded', async () => {
  await retrieveToken(app);
  keepRetrievingToken(app);
});

The end result is that you'll have window.sessionToken available to any of your scripts that need it. It will always be the latest/current session token because it auto-updates every 5 seconds (you can adjust how often it updates).

I've added a hidden input field to all of my forms named "token" that has a class "access-token". I automatically update the value of the input field, and I also set the "Authorization" header in all jQuery AJAX requests. You can do this by adding this to the retrieveToken() function:

// update the access token in forms
$('.access-token:input').val(window.sessionToken);

// update the authorization header sent with ajax requests
$.ajaxSettings.headers['Authorization'] = 'Bearer ' + window.sessionToken;

@squatto and @diemah77 - thanks to you both for sharing!!

gnikyt commented 3 years ago

@squatto So the issue with the forms is CRSF is being regenerated? Any suggestions off-the-bat for this?

squatto commented 3 years ago

@squatto So the issue with the forms is CRSF is being regenerated? Any suggestions off-the-bat for this?

@osiset Yeah that's what's happening. Since every page load is a new login session (right..?) it generates a fresh token, so the token on the form never matches the token in the controller.

Can we use standard cookies/sessions within the app itself, since they rely on first-party cookies? Could you do everything that you're already doing to set/verify the token, and once it's verified, could you check if the shop is already logged in and let it use the same session?

squatto commented 3 years ago

It looks like that is how Sanctum handles same-domain/app SPA authentication, albeit with a little bit of simple configuration.

image

gnikyt commented 3 years ago

I went down this road before when implementing the ITP stuff. Cookies would not persist between requests because they were considered third-party without the ITP flow.

Reintroducing cookies/ITP will get us back to the nightmare people faced before with cookies.

For now, I'll continue on with my work and review. I will also add a small JS helper to refresh the token for non-SPAs on-click of a link/form.

I will come back to the CSRF issue at a later time. Unfortunately my personal man-power to support everything at once is hard, the new cookie policies from browsers really hammered my time and caused issues for others, to ease things, my goal was to remove dependencies on them all together... I'm sure we'll find a workaround.

squatto commented 3 years ago

@osiset That makes sense. I wasn't sure how first/third-party cookies are differentiated and handled within iframes. Is it considered third-party because the domain of the iframe is different than the parent? I was thinking of the app in isolation, not within an iframe.

I just excluded the app routes from CSRF verification for now. It's not the most ideal solution, sure, but it's a calculated risk vs. reward at this point.

I completely understand your position about time and man-power. I can't thank you enough for all of your work on this package, and for the package itself - it is greatly appreciated! I know that the Open Collective monthly isn't much (the Quadra Ecommerce one is me), but I hope it helps in some way, even if it's just a bit of encouragement. I will continue submitting PRs as I am able to, and I'll also send along some strategies/code I've used to make the non-SPA process easier to deal with. Maybe we could throw them on a new wiki page, or something along those lines.

Thank you again!

gnikyt commented 3 years ago

Hey, yeah so the iframe stuff is the exact issue. With ITP flow its:

  1. Load iframe
  2. Check if cookies are OK
  3. If not, escape iframe to fullpage of app, set cookie
  4. Re-check if cookies are OK
  5. If so, load back to iframe

It was quite a convoluted solution which involved both server side and frontend JS to fix.

Disabling CSRF is an option yeah.. given routes protected with the middleware anyways its probably fine.

And yes, thanks! I know people appreciate the package :) I just can't be drowned in cookie issues any longer, which is why I started this branch :) any submitted PRs, assistance is super helpful... community input helps grow!

mohinht commented 3 years ago

@osiset

You can pull from the feature/cookieless branch, but I don't have upgrade instructions yet completed.

Hey, First of all, many thanks to you for creating this awesome package. Also thanks to the all contributor.

I'm building an app using this package. But I'm stuck after changing the authentication module.

I can see, feature/cookieless branch has been added. I'm using react Polaris / app bridge in my app admin.

May I add billing/webhook using this feature/cookieless package?

Or should I wait to release the final update?

Thank you!

raibhuwan commented 3 years ago

i just implemented the feature/cokkieless branch, but there seems to be problem while installing the app, the shopify shop simply redirects to the home page and then the token page, oauth page doesnt appear, i have the app url "https://{shopdomain}/", what should i change it to?

ghost commented 3 years ago

i just implemented the feature/cokkieless branch, but there seems to be problem while installing the app, the shopify shop simply redirects to the home page and then the token page, oauth page doesnt appear, i have the app url "https://{shopdomain}/", what should i change it to?

you can install the app from my thread or override "VerifyShopify" middleware

raibhuwan commented 3 years ago

@Enmaboya Everythings work fine, except the billing part, it gives 500 error, Call to a member function getId() on null on billingcontroller trait

Seems the authorization is negleted in this page

thang12l commented 3 years ago

@Enmaboya Everythings work fine, except the billing part, it gives 500 error, Call to a member function getId() on null on billingcontroller trait

Seems the authorization is negleted in this page

Hi @raibhuwan ,

It may be worth checking if one of your plans has the "on_install" field setting as true since the billing controller will look at the plans table to find a possible plan to bill the user. Or you can try to set "plan_id" of your testing shop to be a valid plan to overcome this error for a while. Another option to try is to remove billable middleware for moving forward if you just want to check the new authentication with token-based.

raibhuwan commented 3 years ago

@Enmaboya Everythings work fine, except the billing part, it gives 500 error, Call to a member function getId() on null on billingcontroller trait Seems the authorization is negleted in this page

Hi @raibhuwan ,

It may be worth checking if one of your plans has the "on_install" field setting as true since the billing controller will look at the plans table to find a possible plan to bill the user. Or you can try to set "plan_id" of your testing shop to be a valid plan to overcome this error for a while. Another option to try is to remove billable middleware for moving forward if you just want to check the new authentication with token-based.

Thank you for your concern. I have all my plans set false for "on_install" and i think the authorization is the main problem. In "VerifyShopify" middleware, i see the code // Continue if current route is an auth or billing route if (Str::contains($request->getRequestUri(), ['/authenticate', '/billing'])) { return $next($request); }

Doesn't it neglect the authorization and auth()->user() returns null, so i removed billing from code and then the billing page works, but at time of payment redirection to https::/{{site_ur;}}/billing/process/?charge_id=123 , the site goes down with 500, and checking the logs , i got error Call to a member function getId() on null on billingcontroller trait

gnikyt commented 3 years ago

I don't think this is worth installing ATM if you're doing it simply to get an app out that meets the new requirements. The unit tests need updating for the new PRs, more physical testing is needed, and CSRF for forms issue addressed yet.

gnikyt commented 3 years ago

@squatto Are you currently working on or expecting to issue out any PRs in the next couple days? If not, I am going to start and adjust the unit tests to test out the latest changes. If you are expecting to send any in, I'll pause that for now :)

squatto commented 3 years ago

@osiset I don't have anything that I'm working on right now, or that I anticipate working on. That being said, I'm changing things as they come up, so I can't guarantee that I won't change anything ;) However, I'm to the point right now that what I have left in my own app to fix is unrelated to this package (XHR requests from third-party packages) so I don't foresee anything big coming up.

gnikyt commented 3 years ago

@squatto Sounds good! Thanks! I will work on the tests.

squatto commented 3 years ago

One thing that I've come across that I think would be beneficial to know for those using this package for a non-SPA app is handling redirects from your controllers. You will quickly learn that redirecting back to your app doesn't work well (or may not work at all) because you no longer have a token and/or shop in the request. To address this, you will have to redirect through the "get a token" route.

Your processes will look like this:

image

By doing it this way, you are able to mimic having a session token available to you during routing.

One important thing to remember is that if you redirect your users directly to your own route and bypass the "get a token route", you can potentially get an error if the package is unable to determine the shop domain from the current request. Also, you end up with one additional request in the chain because the package can't find a token at your route and redirects you through the "get a token" route and back again:

image

To make this simple to handle, I created a macro on Redirect with the same signature as Redirect::route() that redirects the user through the "get a token" route:

This should be added to the boot method of your AppServiceProvider

use Illuminate\Support\Facades\Redirect;
use Osiset\ShopifyApp\Objects\Values\ShopDomain;
use function Osiset\ShopifyApp\getShopifyConfig;

// Add Redirect::appTokenRoute() macro to redirect the user through the "get a token" route.
// It can also be used as redirect()->appTokenRoute()
// The signature is the same as URL::route() / the route() helper
Redirect::macro(
    'appTokenRoute',
    function (string $route, $params = [], bool $absolute = true) {
        return redirect()->route(
            getShopifyConfig('route_names.authenticate.token'),
            [
                'shop'   => ShopDomain::getFromRequest(request()),
                'target' => route($route, $params, $absolute),
            ]
        );
    }
);

To use it in your controllers, just use appTokenRoute() where you would normally use route():

return redirect()->appTokenRoute('settings.payment-method'); // or Redirect::appTokenRoute()

Or with route parameters:

return redirect()->appTokenRoute('products.create', [$productType, $productDraft]);
gnikyt commented 3 years ago

Hmm that's clever workaround. I had a tokenUrl function for use in Blade which appended the token automatically. But since it only lives for a minute, would it be worth including this, so that all non-SPA links go through the get token route?

On Mon., May 10, 2021, 16:39 Scott Carpenter, @.***> wrote:

One thing that I've come across that I think would be beneficial to know for those using this package for a non-SPA app is handling redirects from your controllers. You will quickly learn that redirecting back to your app doesn't work because you no longer have a token in the request. To address this, you will have to redirect through the "get a token" route.

Your processes will look like this:

[image: image] https://user-images.githubusercontent.com/748444/117707880-5a181e80-b18c-11eb-90fa-74df3f9e9b12.png

By doing it this way, you are able to mimic having a session token available to you during routing.

One important thing to remember is that if you redirect your users directly to your own route and bypass the "get a token route", you can potentially get an error if the package is unable to determine the shop domain from the current request. Also, you end up with one additional request in the chain because the package can't find a token at your route and redirects you through the "get a token" route and back again:

[image: image] https://user-images.githubusercontent.com/748444/117709479-35bd4180-b18e-11eb-9fff-1c95278ac769.png

To make this simple to handle, I created a macro on Redirect with the same signature as Redirect::route() that redirects the user through the "get a token" route:

This should be added to the boot method of your AppServiceProvider

use Illuminate\Support\Facades\Redirect;use Osiset\ShopifyApp\Objects\Values\ShopDomain;use function Osiset\ShopifyApp\getShopifyConfig; // Add Redirect::appTokenRoute() macro to redirect the user through the "get a token" route.// It can also be used as redirect()->appTokenRoute()// The signature is the same as URL::route() / the route() helperRedirect::macro( 'appTokenRoute', function (string $route, $params = [], bool $absolute = true) { return redirect()->route( getShopifyConfig('route_names.authenticate.token'), [ 'shop' => ShopDomain::getFromRequest(request()), 'target' => route($route, $params, $absolute), ] ); } );

To use it in your controllers, just use appTokenRoute() where you would normally use route():

return redirect()->appTokenRoute('settings.payment-method'); // or Redirect::appTokenRoute()

Or with route parameters:

return redirect()->appTokenRoute('products.create', [$productType, $productDraft]);

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/osiset/laravel-shopify/issues/744#issuecomment-837168772, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASO4OS36YQPLWVJO5OAHW3TNAVNZANCNFSM4ZLA3ICA .

squatto commented 3 years ago

One other thing that you will need to watch out for is that every request has a new session, so your flash data will not be in the next request. This includes errors you pass with ->withErrors(). You will need to do something else to ensure that your flash data makes it to the redirect URL.

I ended up passing a language key in the notice query param of my redirect URL, watching for it in a service provider, and populating the current request's flash data notice value with the resolved language value of the key:

if (request()->has('notice')) {
    session()->now('notice', __(request('notice')));
}

Example usage:

return redirect()->appTokenRoute('settings', ['notice' => 'settings.messages.settings_saved']);

Then in lang/en/settings.php I have this:

return [
  'messages' => [
    'settings_saved' => 'Your settings have been saved',
  ],
];

The end result is the intended flash notice in my Shopify app:

image

You could do something similar for errors, although you may need to deal with the expected error bag (or just use a single string error in the same way I used notice above)

squatto commented 3 years ago

Hmm that's clever workaround. I had a tokenUrl function for use in Blade which appended the token automatically. But since it only lives for a minute, would it be worth including this, so that all non-SPA links go through the get token route?

I absolutely would. tokenUrl() doesn't have any purpose because (as you said) the token is likely expired. You either redirect to the "get a token" route like I did, or the package is going to do it for you. Either way you're going to end up having to get a new token. I did it the way I did so that I could avoid the extra token check + redirect.

gnikyt commented 3 years ago

Solid. I'll integrate your macro into the package then! But yeah with the session flashes.. all we can do I guess, trade offs.

On Mon., May 10, 2021, 16:55 Scott Carpenter, @.***> wrote:

Hmm that's clever workaround. I had a tokenUrl function for use in Blade which appended the token automatically. But since it only lives for a minute, would it be worth including this, so that all non-SPA links go through the get token route?

I absolutely would. tokenUrl() doesn't have any purpose because (as you said) the token is likely expired. You either redirect to the "get a token" route like I did, or the package is going to do it for you. Either way you're going to end up having to get a new token. I did it the way I did so that I could avoid the extra token check + redirect.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/osiset/laravel-shopify/issues/744#issuecomment-837193512, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASO4OXSLOIH5P34PZTTU6LTNAXKTANCNFSM4ZLA3ICA .

squatto commented 3 years ago

Solid. I'll integrate your macro into the package then! But yeah with the session flashes.. all we can do I guess, trade offs.

Sounds great! I'll keep an eye out for it and remove it from my code.

Once I added the error/notice lang handling, it became easy to deal with the flash data. Much better than adding ITP back in! πŸ˜‰

gnikyt commented 3 years ago

@squatto Thanks, I've incorporated your macro into the package, with an additional one for non-redirects. Appears to work as it should ATM.

Moving on to update the unit tests!

squatto commented 3 years ago

@osiset Excellent, thank you! Thanks for handling the method rename as well, I appreciate it.

As I look through your changes, I realize just how much I've gotten used to being lazy and using the arg-free helpers to get the class instance (request(), session(), etc.) I honestly had no idea that Request::instance() was a thing πŸ€¦πŸΌβ€β™‚οΈ

I'm digging invokable classes though. That makes macro registration super clean. I need to start using those more.

gnikyt commented 3 years ago

I'm not a total fan of the facades, it's not much different than global functions (as session() is for example). But I've found previously a few years ago that there was times I need facades, and having a mix of facades and the global helpers just didn't sit right, so I decided to go "all-in" on facades rather than mix.

gnikyt commented 3 years ago

Unit tests are fixed for the new PRs recently from everyone.

Going to review the coverage, clean up, do more physical testing, etc.

squatto commented 3 years ago

I think that you hit the nail on the head: consistency is key. As with most things in the dev world, it's ultimately a matter of preference.

gnikyt commented 3 years ago

Totally.

So in physical testing... things seem to be working out fine so far.

Things to work on:

  1. Forms... so yes, they only work when you disable CSRF, probably our only option for non-SPAs, I will add a JS function to update the token before submission.
  2. Per-user auth... was partly removed in this branch, will be enabling it and coming up with a way to re-integrate that feature

Other than that, I don't think theres anything major outstanding ATM off the top of my head.

squatto commented 3 years ago

@osiset That is great to hear! I can't think of anything major either. I think I missed per-user auth in the pre-cookieless process, so I'm not terribly concerned about that...for myself πŸ˜‰

If it helps, for the form handling, I created a blade directive @sessionToken that I always output right after @csrf:

Blade::directive('sessionToken', function () {
    return '<input type="hidden" class="session-token" name="token" />';
});

It has the session-token class so that it's easy to update with JS, even if there are multiple forms on the same page. I changed the JS retrieveToken() function to do this (this is jQuery, but easily adapted to vanilla JS/etc.):

async function retrieveToken(app) {
  window.sessionToken = await utils.getSessionToken(app);

  // update the session token in forms
  $('.session-token:input').val(window.sessionToken);

  // update the authorization header sent with ajax requests
  $.ajaxSettings.headers['Authorization'] = 'Bearer ' + window.sessionToken;
}

You could definitely dynamically create the "token" field on form submits so that you don't have to remember to add @sessionToken to your forms.

That also keeps the XHR auth header updated, in case you're using jQuery's ajax methods. If you're using axios then it's just as easy:

window.axios.defaults.headers.common['Authorization'] = 'Bearer ' + window.sessionToken;

(that assumes that you're using window.axios, which is what Laravel does by default in /resources/js/bootstrap.js)

gnikyt commented 3 years ago

Perfect! I'll integrate that, that should get us close to the finish line.

squatto commented 3 years ago

What would you think about changing Redirect::tokenRedirect() to Redirect::tokenRoute()? It's a wrapper around Redirect::route(), so it feels more semantically correct. "Redirect to the token route" vs. "Redirect to the token redirect".

I realize the URL::tokenRoute() macro class is named TokenRoute, but you could use RedirectTokenRoute and UrlTokenRoute (or whatever) to separate them.

gnikyt commented 3 years ago

Sounds fine to me

On Wed., May 12, 2021, 17:31 Scott Carpenter, @.***> wrote:

What would you think about changing Redirect::tokenRedirect() to Redirect::tokenRoute()? It's a wrapper around Redirect::route(), so it feels more semantically correct. "Redirect to the token route" vs. "Redirect to the token redirect".

I realize the URL::tokenRoute() macro class is named TokenRoute, but you could use RedirectTokenRoute and UrlTokenRoute (or whatever) to separate them.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/osiset/laravel-shopify/issues/744#issuecomment-840059503, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASO4OVPGIGORKIA6BYNEXDTNLNDPANCNFSM4ZLA3ICA .

squatto commented 3 years ago

OK awesome. Do you want me to PR the change or do you want to make it? I'm happy to PR it if it would help.

davodavodavo3 commented 3 years ago

Hi guys, I tested cookieless branch. What is causing new 3 sessions generation for each app visit?

The custom package sessions manager?

squatto commented 3 years ago

@osiset I went in to fix an issue with ShopDomain::fromRequest() when there isn't a query string and noticed that you already fixed it. However, there's one other small change I was going to PR, but it's hardly enough to PR by itself so I'll just let you know what it was.

The parseQueryString() signature needs to be changed from string $qs to ?string $qs to allow it to accept a null. The method itself is already written to accept a null value:

$split = preg_split($d ? ($COMMON_SEP[$d] || '/['.$d.']\s*/') : $DEFAULT_SEP, $qs ?? '');

My PR was going to change the docblock and the signature to this:

/**
 * Parse query strings the same way as Rack::Until in Ruby. (This is a port from Rack 2.3.0.).
 *
 * From Shopify's docs, they use Rack::Util.parse_query, which does *not* parse array parameters properly.
 * Array parameters such as `name[]=value1&name[]=value2` becomes `['name[]' => ['value1', 'value2']] in Shopify.
 * See: https://github.com/rack/rack/blob/f9ad97fd69a6b3616d0a99e6bedcfb9de2f81f6c/lib/rack/query_parser.rb#L36
 *
 * @param string|null $qs The query string.
 * @param string|null $d The delimiter.
 *
 * @return mixed
 */
function parseQueryString(?string $qs, ?string $d = null): array
{
  // ...
}

I also changed string $d = null to ?string $d = null so that it's explicitly nullable.

squatto commented 3 years ago

Hi guys, I tested cookieless branch. What is causing new 3 sessions generation for each app visit?

The custom package sessions manager?

@davodavodavo3 Where are you seeing 3 new sessions? I added a log call to \Illuminate\Session\Store::start() and I'm only seeing 1 hit on a single page load.

I'm wondering if what you're seeing it the result of the redirection when the token isn't present:

davodavodavo3 commented 3 years ago

Screenshot from May 13, 2021 1_08 AM

So if I go back and open the app again I will get another 3 sessions generated. If I refresh the page again 3 sessions. If click in on the hello, again 3 sessions generated

davodavodavo3 commented 3 years ago

So in few seconds the session storage can increase with a bunch of session if you have few many shops. image