FIX: /auth redirects to / endpoint even if custom prefix setup

[dooman87]

Description

Fixes (issue #) #1148

The issue is marked as fixed however the prefix still doesn't work if https://APP_HOST/ doesn't return 302 to auth endpoint.

Type of change

• koa-shopify-auth: Patch: Bug

[dooman87]

@TheMallen @alexandcote @vsumner @ismail-syed Could you please have a look at this one please? Really appreciate that.

[ismail-syed]

I believe we'll still need the / at the beginning. Can we update the default prefix in the function argument to be '/' instead of ''?. Users who set a prefix will need to make sure the use a / (eg. /my-prefix)

[marutypes]

👆

[dooman87]

You are right, sorry. I've updated PR, so it will use '/' path in case there is no prefix setup. Thanks a lot for reviewing this!

[ismail-syed]

I should have been more clear earlier, sorry. I meant in line 2 we can make the following change

- const requestStorageAccess = (shop: string, prefix = '') => {
+ const requestStorageAccess = (shop: string, prefix = '/') => {

[dooman87]

gotcha, less conditions :) I made a change, please have a look

[ismail-syed]

Last nit: please add a link to the PR at the end of this like (eg. [[#PR_NUMBER]](LINK))

[dooman87]

Cheers, done

[dooman87]

@ismail-syed Could we merge this one please?

[ismail-syed]

@dooman87 merged, sorry for the delay!

[seamusabshere]

this causes https://github.com/Shopify/quilt/issues/1550

[dooman87]

this causes #1550

Argh, you are right, sorry. I didn't wait until release and used my fork without latest changes to default value :(

[simonhaenisch]

From #1428 (comment):

I don't think we should set prefix here. The path that the application lives on is orthogonal to the paths that the OAuth logic lives on. (You can tell this because the verifyRequest middleware can be applied separately from the shopifyAuth middleware.)

In a separate PR, however, I'd be open to a couple alternatives:

1. A new applicationUrl configuration option that would specify the path to direct the user when cookie access is enabled.
2. Logic to pass a returnTo value through query params. (We do this in shopify_app, though it's pretty complex IIRC.)

and #1440 (comment):

I agree with @ragalie that the prefix shouldn’t apply to appTargetUrl. E. g. my prefix is /api for all the back-end routes (requirement of Next.js API routes), but the front-end app lives under / and sending the user to /api would result in a 404.

Not that it bothers me greatly because I'm maintaining my own copy of koa-shopify-auth now, but you might want to re-consider this "fix".

[dooman87]

From #1428 (comment):

I don't think we should set prefix here. The path that the application lives on is orthogonal to the paths that the OAuth logic lives on. (You can tell this because the verifyRequest middleware can be applied separately from the shopifyAuth middleware.)
In a separate PR, however, I'd be open to a couple alternatives:

1. A new applicationUrl configuration option that would specify the path to direct the user when cookie access is enabled.
2. Logic to pass a returnTo value through query params. (We do this in shopify_app, though it's pretty complex IIRC.)

and #1440 (comment):

I agree with @ragalie that the prefix shouldn’t apply to appTargetUrl. E. g. my prefix is /api for all the back-end routes (requirement of Next.js API routes), but the front-end app lives under / and sending the user to /api would result in a 404.

Not that it bothers me greatly because I'm maintaining my own copy of koa-shopify-auth now, but you might want to re-consider this "fix".

I see where you are coming from and I think having two separate prefixes would be a good solution. However, I think using prefix in appTargetUrl is better cause if your app is deployed under that prefix and you only have control over that prefix then you still could workaround the problem using redirects.

[simonhaenisch]

However, I think using prefix in appTargetUrl is better

Not sure how this is better than being able to configure api and app prefix separately? Or do you mean it's better than not applying any prefix to appTargetUrl? I get your point, I can create api/index.ts and redirect to /.

It's just that this is a breaking change not just a "fix", because I'm using a prefix for the api only (/api) and if I updated this patch version, it would suddenly break my app 😐 (which is just another reason I'm not confident to use this package yet and rather maintain a copy)

Also, if I used a prefix the way you are, I'd still need two separate prefixes, e. g. /shopify and /shopify/api.

[dooman87]

However, I think using prefix in appTargetUrl is better

Not sure how this is better than being able to configure api and app prefix separately? Or do you mean it's better than not applying any prefix to appTargetUrl? I get your point, I can create api/index.ts and redirect to /.

It's just that this is a breaking change not just a "fix", because I'm using a prefix for the api only (/api) and if I updated this patch version, it would suddenly break my app 😐 (which is just another reason I'm not confident to use this package yet and rather maintain a copy)

Also, if I used a prefix the way you are, I'd still need two separate prefixes, e. g. /shopify and /shopify/api.

Yup, you would still need two separate routes for /app-prefix and /app-prefix/auth.

I think you are right and we did need to bump major version in this case. Sorry, I thought it's just a bug cause saw a ticket and then PR you mentioned above that's been closed because of big refactor. That was my first app and first experience with next.js and this package. Turned out to deploy the whole thing under prefix is quite challenging and I regret not using subdomain in the first place :)

[simonhaenisch]

Turned out to deploy the whole thing under prefix is quite challenging

https://nextjs.org/blog/next-9-5#customizable-base-path