-
-
Notifications
You must be signed in to change notification settings - Fork 377
Support for new JWT auth flow #551
Comments
Hi @Kyon147 @osiset, What would you say on this? Any idea how can we implement in the package? |
From the last time I looked into this, the JWT flow does not 100% help with the cookies that are more on the Laravel side as this sits more around authenticating with app bridge / Shopify. @osiset would know a lot more than myself - no doubt he is probably looking at potential solutions. |
JWT doesn't solve the issue no, it's made for SPAs. Cookie issue something I'm still trying to solve. |
For someone interested in implementing JWT session token based authentication, I have implemented it in one of my own project. Here's the commit: niveshsaharan/shark@1bd88f7 |
@niveshsaharan I currently have no time to implement a new auth flow to support JWT. Would you mind creating a guard PR for this? If not I can revisit at a later date. |
@osiset I also don't have any time because Shopify has banned all of my apps and now I'm in survival mode. You might have heard about it already and if you haven't, you can read here: |
No worries. Since this is for SPAs anyways, it will be lower priority on my list. Cookie is the top right now. Thanks! |
I've been working on this. I've got it all working, and am at the point where I can do 2 things, but am unsure if I should do them in this package or not.
I'm at a point where I could probably remove 95% of the Cookie code and things will still work. This will resolve a massive amount of issues we've all experienced, including #443 #522 #531 and this one (#551) - resolves problems like being logged into 2 shops at once on the same browser and having them affect each others data. The only place I'm still using Cookies here is with the initial signup and billing, but we could probably get rid of that completely - however, Shopify documentation has no info on how to do this with the new token flow. To bring this functionality in means I would have to:
I've got a private project where I converted this package to work with an SPA (with it's own JWT setup) - I could reuse a bunch of this to quickly convert this package into being SPA by default. However I do not personally use the official shopify React SPA. I much prefer Vue, and my implementation is Vue based. Pragmatically speaking, the bulk of the community behind this app will be using React and the official Polaris packages, and I don't want to do a bunch of work getting Vue into place only to have it be rejected. In that light I'm thinking of making a barebones version of the JWT functionality, and let it live side-by-side with Cookies for now, just to get it out there for the everyone else to try. We could include this JWT in the next big release as a stipped down optional feature, iron out the kinks and eventually make it the default, replacing the need for Cookies when interacting with the app from any frontend (besides for initial auth and billing selection). Another option is, apparently, TurboLinks. I haven't looked into it. So I need to know - @osiset @Kyon147 @lucasmichot - how should we proceed? Tyler, is there a way for us to discuss this offline? |
I'm not sure if need to create a guard for this. What do you think about just adding a method in public function handle(Request $request, Closure $next)
{
if($this->tryLoginWithShopifyJwtToken($request))
{
return $next($request);
}
// rest of code ....
} private function tryLoginWithShopifyJwtToken(Request $request): bool
{
if( $request->bearerToken())
{
$exploded = explode('.', $request->bearerToken(), 3);
if (3 == \count($exploded)) {
list($header, $payload, $signature) = $exploded;
$payloadArray = json_decode(base64_decode($payload), true);
if ($payloadArray['exp'] > now()->utc()->timestamp
&& $payloadArray['nbf'] <= now()->utc()->timestamp
&& \Illuminate\Support\Str::startsWith($payloadArray['iss'], $payloadArray['dest'])
) {
$shopDomain = \Osiset\ShopifyApp\Objects\Values\ShopDomain::fromNative($payloadArray['dest']);
if (! $shopDomain->isNull()) {
$shop = resolve(\Osiset\ShopifyApp\Contracts\Queries\Shop::class)->getByDomain($shopDomain);
if ($shop) {
// Generate api helper from shop
$apiHelper = $shop->apiHelper();
$apiSecret = $apiHelper->getApi()->getOptions()->getApiSecret();
$hmacLocal = rtrim(strtr(base64_encode(hash_hmac('sha256', $header.'.'.$payload, $apiSecret, true)), '+/', '-_'), '=');
if (hash_equals($hmacLocal, $signature)) {
return $this->loginShop($request, $shopDomain);
}
}
}
}
}
}
return false;
} |
@niveshsaharan this is a great starting point @darrynten yes, it would solve a world of problems but leave the package only supporting an SPA would limit, hmm, yes please email me! Seems GitHub discussions is not active feature yet :/ |
@osiset where should I send the mail to? |
Why not just using https://github.com/tymondesigns/jwt-auth ? |
Thats a good option to try. |
@lucasmichot @osiset I oppose the usage of that package for this basic functionality. I think most SPAs will be authenticating with their own API endpoints using this package exact JWT package for non-Shopify related things in order to guard endpoints that do not interact with Shopify. An example is when an app interacts with a 3rd party like a shipping company. This use case requires a stateless API be guarded to confirm the user is who they say they are before interacting with non-Shopify entities on the backend, and the most common way of doing this is with the tymon package. Using this package directly when we only need like 5% of the functionality may hinder applications that build on top of this package. |
I've completed my work on this and am currently adding additional unit tests. Will open a PR into this repo as soon as all checks are green. |
Update: Figuring out why tests run locally but not on CI. Should be done soon. |
I've opened up PR #601 which contains the JWT functionality. |
Currently in review! Thanks @darrynten |
Are there anyone working on support for the new auth flow using JWT instead of cookies?
Ref: https://shopify.dev/tools/app-bridge/authentication
The text was updated successfully, but these errors were encountered: