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

Fix Issues with EnsureAuthenticatedLinks and updated OAuth flow #1549

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

rdillensnyder
Copy link
Contributor

@rdillensnyder rdillensnyder commented Oct 25, 2022

What this PR does

The 'embedded' param from the updated OAuth flow is lost by the EnsureAuthenticatedLinks concern during redirects for deep linked pages or Admin extensions. Adding this param to ensure the flow completes successfully.

Reviewer's guide to testing

Directly link to a page for an app using the EnsureAuthenticatedLinks concern such as Order Printer

Checklist

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users
  • Update README.md, if appropriate.
  • Update any relevant pages in /docs, if necessary
  • For security fixes, the Disclosure Policy must be followed.

Sorry, something went wrong.

@rdillensnyder rdillensnyder force-pushed the authenticatedlinks-patch branch from 9bc90cd to 2cd4937 Compare October 25, 2022 17:51
@andyw8 andyw8 force-pushed the authenticatedlinks-patch branch from 2cd4937 to 336f6d4 Compare October 25, 2022 18:24
@rdillensnyder rdillensnyder force-pushed the authenticatedlinks-patch branch from 336f6d4 to 9142cfd Compare November 4, 2022 14:50

Unverified

The email in this signature doesn’t match the committer email.
@rdillensnyder rdillensnyder force-pushed the authenticatedlinks-patch branch from 9142cfd to 7d7df7a Compare November 4, 2022 14:52
@andyw8 andyw8 merged commit feced71 into main Nov 4, 2022
@andyw8 andyw8 deleted the authenticatedlinks-patch branch November 4, 2022 14:54
@rdillensnyder rdillensnyder linked an issue Nov 4, 2022 that may be closed by this pull request
@netwire88
Copy link

Thanks all, looking forward to this fix.

nelsonwittwer pushed a commit that referenced this pull request Nov 12, 2022

Unverified

The email in this signature doesn’t match the committer email.
nelsonwittwer added a commit that referenced this pull request Dec 7, 2022
* session stored in rails instead of library

* move responsiblity for session persistence

* move load_current_session to login_protection

* FIXME notes

* Move session concerns back to session utils

* Add `embedded` param to `splash_page` (#1549)

* use online tokens appropriately if there is a user session storage available

* move load_current_session to login_protection

* Move session concerns back to session utils

* refactor login protection and tests with sesison ownership change

* rubocop'd

* no more session_storage in generated example

* add x86_65-linux platform for CI

* no more local pointer to api lib

* more gemlock

Co-authored-by: rdillensnyder <[email protected]>
Co-authored-by: Teddy Hwang <[email protected]>
fabriazza pushed a commit to fabriazza/shopify_app that referenced this pull request Feb 1, 2023
fabriazza pushed a commit to fabriazza/shopify_app that referenced this pull request Feb 1, 2023
…ify#1563)

* session stored in rails instead of library

* move responsiblity for session persistence

* move load_current_session to login_protection

* FIXME notes

* Move session concerns back to session utils

* Add `embedded` param to `splash_page` (Shopify#1549)

* use online tokens appropriately if there is a user session storage available

* move load_current_session to login_protection

* Move session concerns back to session utils

* refactor login protection and tests with sesison ownership change

* rubocop'd

* no more session_storage in generated example

* add x86_65-linux platform for CI

* no more local pointer to api lib

* more gemlock

Co-authored-by: rdillensnyder <[email protected]>
Co-authored-by: Teddy Hwang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EnsureAuthenticatedLinks doesn't understand deep linked pages
3 participants