-
Notifications
You must be signed in to change notification settings - Fork 221
[koa-shopify-auth] Fixes ITP 2.3 and Safari 13.1 enable cookies loop #1413
Conversation
36730c1
to
f6e7609
Compare
f6e7609
to
16264cd
Compare
8dcff18
to
f187df4
Compare
Can you please add a changelog update for this package. |
c078324
to
6377ef1
Compare
@@ -7,6 +7,10 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). | |||
|
|||
<!-- ## [Unreleased] --> | |||
|
|||
## [3.1.57] - 2020-05-01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a 3.1.61
since the last released version was 3.1.60. The changelogs don't keep a good track of peer dep bumps via lerna 😢
Co-authored-by: Mike Ragalie <mike.ragalie@shopify.com> Co-authored-by: Tim Anema <tim.anema@shopify.com>
6377ef1
to
96d7ee4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, glad to hear it was already tophatted by some folks. Left a few comments mostly as documentation for stuff we might want to make nicer in the future.
@@ -0,0 +1,43 @@ | |||
// Copied from https://github.com/Shopify/shopify_app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably fine to leave this as is (especially because it is copied from somewhere), but if we felt like making it a bit more readable we could actually have this as TypeScript with it's own tsconfig that compiles it for direct script tag use if we wanted. Maybe a good future improvement.
return; | ||
} | ||
|
||
ctx.body = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we might want to introduce tagged template literals or something so we can make all of these a bit nicer, but this is fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!document.cookie) { | ||
throw 'Cannot set third-party cookie.' | ||
} | ||
this.redirectToAppTargetUrl(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ayronshopify @ismail-syed @TheMallen
I'm a Shopify partner who's trying to understand this authorization process and I'm confused about this line and was hoping to get some clarification.
Why do we redirect the user to this.redirectData.appTargetUrl
(which was initialized as /?shop=${shop}
) rather than /auth
, which is the oAuthStartPath
in the createShopifyAuth
function?
The logic that checks the shopify.granted_storage_access
cookie is associated with the /auth
path and so it seems to me that we should be redirecting the user to /auth
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Good question. I think the reason is that we may have already completed OAuth. If we've already completed OAuth, and now we also have storage access, then the app is ready to load, so it doesn't make sense to do OAuth again.
If we haven't completed OAuth yet, then when we hit the app target URL we'll realize we need to do OAuth, and then redirect to OAuth anyway, so that case is also covered.
Does that make sense?
The redirect always goes to "/auth/..." without the original prefix. We set the prefix property, and the redirect causes a 404. By prefix, we mean we use the "prefix" in the example provided for koa-shopify-auth...
So in the example above, on safari, it redirects to "/auth/enable_cookies..." instead of "/shopify/auth/enable_cookies...", and thus shopifyAuth doesn't serve that non-prefixed route. It would appear these are hard-coded client-side. For instance, in client/request-storage-access.ts:
In the past we could use the referer (sic) to retrieve the original path, but now that this redirect happens client side (client/request-storage-access.ts), we no longer receive/have access the referer in order to patch this up. Any way to preserve or send along the prefix? |
Description
Package: koa-shopify-auth
Fixes #1387
Intelligent Tracking Prevention (ITP) prevents websites from tracking users and with Safari 13.1 blocking all third-party cookies by default, we're finding that users are experiencing an endless loop of seeing the
Enable Cookies
screen, clicking the button, and being redirected back to theEnable Cookies
screen again. This is due to the fact that we are not currently using the Storage Access API by callingdocument.requestStorageAccess()
which is the way to ensure these cookies can now be saved/accessed.Type of change
We are using some of the code implemented by
shopify_app
as this is already working and thoroughly tested there.We're keeping the
Enable Cookies
page which is surfaced at the top-level:Once the user clicks the button they will be redirected to a new page in an iframe which will make the
document.requestStorageAccess()
call needed to save the cookies.Older browsers/Chrome will probably not see either of these screens, depending on the version checking done in the
StorageAccessHelper
. Newer Firefox users will see theEnable Cookies
screen. Newer Safari users will see both theEnable Cookies
screen and theBrowser
screen. Once the flow has been completed Older/Chrome/Firefox users shouldn't see any other screens when accessing their apps. However, newer Safari users will always see theBrowser
screen before they can access the app.New Safari Flow:
Checklist