Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

[koa-shopify-auth] [WIP] ITP 2.1 #416

Closed
wants to merge 10 commits into from
Closed

[koa-shopify-auth] [WIP] ITP 2.1 #416

wants to merge 10 commits into from

Conversation

tylerball
Copy link
Contributor

@tylerball tylerball commented Nov 20, 2018

🎩 instructions

  1. checkout this branch locally and yarn link
  2. you can follow the app_builder instructions to get an app set up quick.
  3. in your project yarn link @shopify/koa-shopify-auth

TODO

  • update illustrations
  • clean up names of view methods "create" doesn't seem right.
  • tests for template views
  • fence off mobile and POS user agents.
  • figure out how to get and pass app name through to templates.
  • tests for top level redirect
  • client side tests

@@ -0,0 +1,45 @@
(function() {
function setCookieAndRedirect() {
document.cookie = 'shopify.granted_storage_access=true';

Choose a reason for hiding this comment

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

I just thought of this question now, but should have thought of this earlier: Should the cookies we're setting for both the ITP 2.0 and ITP 2.1 workflows be unique to the app being authenticated (e.g., by adding the app ID to the cookie name)? This question also applies to the shopify_app gem

cc @ragalie

Choose a reason for hiding this comment

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

Nevermind, I was having a brain fart moment. Talked it through with Mike and this shouldn't be an issue. Sorry!

@francinen
Copy link

I'm getting this error when I'm redirected to the app's TLD:
screen shot 2018-11-21 at 10 23 23 am

@tylerball tylerball force-pushed the itp-2.1 branch 3 times, most recently from dc0a1c8 to 2fc7e31 Compare November 22, 2018 21:30
@francinen
Copy link

Hey @tylerball! Does this PR only implement the solution for ITP 2.0? I'm only seeing the Enable cookies view, which is meant for the version of ITP that was released in September and only works for Safari 12.0.

@tylerball tylerball force-pushed the itp-2.1 branch 2 times, most recently from 0e656be to d18056c Compare November 27, 2018 19:53
@tylerball tylerball force-pushed the itp-2.1 branch 3 times, most recently from 090c9bf to e08513a Compare January 2, 2019 22:26
Copy link
Contributor

@marutypes marutypes left a comment

Choose a reason for hiding this comment

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

Is this ready to be reviewed? Left a couple comments but wasn't sure.

@@ -0,0 +1,45 @@
(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why we can't write this in typescript. If you stick another tsconfig.json in the client directory you should be able to get it to compile in place and won't have to change your script tags.

await enableCookiesRedirect(ctx);
return;
}

if (ctx.path === oAuthStartPath && shouldRequestStorage(ctx)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

at this point we have a lot of routes in here, we might start to get value from using koa-router and just exporting the router.routes() middleware.

@tylerball
Copy link
Contributor Author

@TheMallen not entirely, I'm still working on tests but thanks for the feedback. I'm trying to cover as much as I can in request-storage.js and the rest of the client JS without having to actually run anything in a browser environment.

@MasjZam
Copy link

MasjZam commented Mar 28, 2019

We're looking to send out some partner comms warning them of the upcoming changes for Safari 12.1+ users with ITP 2.1

Was wondering if we have a timeline on this PR so we can include it in the comms as well

@lemonmade
Copy link
Member

@tylerball can this either be updated or closed?

@ayronshopify
Copy link
Contributor

ayronshopify commented May 1, 2020

Closing this in favour of #1413

@BPScott BPScott deleted the itp-2.1 branch May 22, 2021 00:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants