Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Simplify the auth flow by removing the full page redirect. #1075

Merged
merged 7 commits into from
Apr 27, 2022

Conversation

Kyon147
Copy link
Collaborator

@Kyon147 Kyon147 commented Feb 4, 2022

This PR looks to reduce the complexity around the Auth flow but keep the same functionality.

it looks to hopefully solve the issue here around #1073 that because of the "auth/token" coming first before the oAuth it fails the Shopify public app requirements.

By just redirecting to the pure URL which is built it removes the need to use the full page redirect.

@stevesweets
Copy link
Contributor

This makes a dramatic positive difference to the auth flow.

@Kyon147
Copy link
Collaborator Author

Kyon147 commented Apr 27, 2022

Hey @osiset - as you are very busy, do you want me to merge this into master and create a new release. This should help a lot of users who are currently getting rejected from Shopify because of the "auth/token" redirect creates in between OAUTH which is against their guidelines.

Let me know and i can get this out today and set up a patch release.

@gnikyt
Copy link
Owner

gnikyt commented Apr 27, 2022 via email

@Kyon147 Kyon147 merged commit 8eb0181 into master Apr 27, 2022
@stevesweets
Copy link
Contributor

stevesweets commented Jul 28, 2022

@Kyon147 i think i've discovered an issue here.

If

  • you've got an embedded app
  • you need to change the scopes

The redirect fails with the old classic https://yyyyyyy.myshopify.com refused to connect, and in the console

Refused to display 'https://yyyyyyy.myshopify.com/' in a frame because it set 'X-Frame-Options' to 'deny'.

... as we know, and as is referenced here: https://community.shopify.com/c/shopify-apis-and-sdks/changing-scope-x-frame-options/td-p/1193972

Obviously, because if you are in an iframe, one cannot simply:

// Just return them straight to the OAUTH flow.
return Redirect::to($result['url']);

Any ideas?

@Kyon147
Copy link
Collaborator Author

Kyon147 commented Jul 28, 2022

Yeah I noticed the same issue, we need to move back to the full page redirect in this instance much like when you change the api scopes the same issue happens.

There's another auth PR which needs to be merged #1097 as this fixes the other issue bust sorts out the iframe redirect one too by moving it back as before.

I'd move back to v17.1.0 for now.

I'll create a hotfix for the current release to remove my PR but keep the other fixes.

@stevesweets
Copy link
Contributor

stevesweets commented Jul 28, 2022

This works, but is there a particular reason it shouldn't be done this way?

return View::make('shopify-app::auth.fullpage_redirect', [ 'authUrl' => $result['url'] ]);

``

    <title>Redirecting...</title>

    <script type="text/javascript">
        window.top.location.href = '{{ $authUrl }}'
    </script>
</head>
<body>
</body>
``

@stevesweets
Copy link
Contributor

@Kyon147 apologies, but this seemed to miss your attention. Do you have any thoughts on if the above would be acceptable?

@Kyon147
Copy link
Collaborator Author

Kyon147 commented Aug 11, 2022

Hey @stevesweets

It's sort of the same result as #1097 but this PR uses the AppBridge functions which is probably a little more inline with Shopify if they ever come back on a App Review.

Your way would work in the same essence as the fullpage_redirect view does effectively the same thing.

@stevesweets
Copy link
Contributor

stevesweets commented Aug 11, 2022

Hey @stevesweets

It's sort of the same result as #1097 but this PR uses the AppBridge functions which is probably a little more inline with Shopify if they ever come back on a App Review.

Your way would work in the same essence as the fullpage_redirect view does effectively the same thing.

Ok nice, thanks for the response :)

@talktohenryj
Copy link

I've been reading through this. So does this mean if I upgrade to 17.2.0 my app should automatically send merchants to reauthenticate if I change the scopes?

I ask because I am adding a new scope and when I add it my test store does not automatically reauth. I am currently on 17.1, I am wondering if upgrading to 17.2 will fix my issue. It sounds like it will.

@Kyon147
Copy link
Collaborator Author

Kyon147 commented Sep 21, 2022

Hey @talktohenryj - yeah upgrading to 17.2 should fix that issue for you.

@talktohenryj
Copy link

hmmm, i did the update but still have the issue. Do I need to be on a specific PHP version? I am currently running 7.4 on dev and production.

@stevesweets
Copy link
Contributor

hmmm, i did the update but still have the issue. Do I need to be on a specific PHP version? I am currently running 7.4 on dev and production.

it may be relevant, it may not - but, it will only reauth when the current session ends.

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.

4 participants