Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the enable_cookies route redirect to ${prefix}/auth/inline #469

Open
wants to merge 1 commit into
base: master
from

Conversation

@beppu
Copy link

commented Jan 16, 2019

Previously, the /auth/enable_cookies route redirected to:

https://${window.shopOrigin}/admin/apps/${window.apiKey} 

which seems useless, because all it ever does is 404. This results in the first installation attempt failing.

GET /shopify/auth?hmac=b7d85bd1976f2f4e3d2badab0473ed47d456855f199cc15917470f1da04ed213&shop=dbap-dev.myshopify.com&timestamp=1547106686 200 2.777 ms - 802
GET /shopify/auth/enable_cookies?shop=dbap-dev.myshopify.com 200 1.664 ms - 10350

However, the /auth/enable_cookies route does set the shopifyTestCookie so the second time around and ever subsequent time afterward, the installation works. Also, notice that subsequent installation attempts go to /auth/inline instead.

GET /shopify/auth?hmac=cc729de08ca82c5bb06e28872e4fc23b0a396678c2af9ec9e95b90e5b0619e37&shop=dbap-dev.myshopify.com&timestamp=1547106888 200 1.463 ms - 786
GET /shopify/auth/inline?shop=dbap-dev.myshopify.com 302 1.916 ms - 619

Solution

What this pull request implements is a change to the /auth/enable_cookies route that redirects to /auth/inline. This makes installation ALWAYS work on the first try and every subsequent try. Let me know if this seems like a reasonable change, @ragalie ?

Make the enable_cookies route redirect to ${prefix}/auth/inline
Previously, it redirected to /admin/apps/${window.apiKey} which seems useless,
because all it ever does is 404.
@ragalie

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

Thanks! I'm not sure why it wasn't something that we caught earlier...

I think this would work, but does more than it needs to. The existing code was redirecting to the application root URL in the embedded context. I think we'll get the same end result (but always working) by redirecting to the application root URL outside the embedded context.

For this to work, we'd either need to redefine prefix to mean the application root (and then the auth stuff will always be mounted at /auth from the application root) or add a new option applicationPath. Then we can redirect to that value.

I haven't tried that, but I think that would be a better way of going about this, since it avoid an extra OAuth flow in some cases.

@beppu

This comment has been minimized.

Copy link
Author

commented Feb 1, 2019

Apologies for the late reply.

The existing code was redirecting to the application root URL in the embedded context. I think we'll get the same end result (but always working) by redirecting to the application root URL outside the embedded context.

As far as I can tell, this doesn't seem to be true. The application root URL is already being used OUTSIDE of the embedded context on the first try (before the shopifyTest cookie exists), and it fails. I presume it's because the application root URL is not yet valid for an uninstalled app.

We start here. Make sure all the cookies for the app's hostname are cleared first.

partners

The first try uses the application root URL outside of the embedded context and it fails presumably because the app isn't installed yet, and it's not yet meaningful.

fail

If we try the same thing again from the partners admin, it works as intended, because the shopifyTest cookie exists, and it takes a different code path that does the right thing.

second try


There may be a more efficient way than going to /auth/inline and redirecting again, but I don't think the application root URL can help us before the app is fully installed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.