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.

Sign up for GitHub

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

Jump to bottom

Process embedded parameter to optimize OAuth #1483

Merged
merged 22 commits into from
Aug 22, 2022
Merged

Process embedded parameter to optimize OAuth #1483

merged 22 commits into from
Aug 22, 2022

Conversation

mkevinosullivan
Copy link
Contributor

@mkevinosullivan mkevinosullivan commented Aug 2, 2022 •
edited
Loading

What this PR does

We want to improve OAuth experience by:

  • Making it faster
  • Reducing the amount of screen flicker.

Note, this PR will require Shopify/shopify-api-ruby#1002 to be merged first, the TODO's updated accordingly, before it can be merged itself

Fixes https://github.com/Shopify/first-party-library-planning/issues/393

Reviewer's guide to testing

To be documented

Things to focus on

  1. Top-level: App installation from the app store
  2. Top-level: Token refresh scopes change (grant screen present)
  3. Top-level: Token refresh no scopes change (grant screen skipped)
  4. Iframe: Token refresh scopes change (grant screen present)
  5. Iframe:Token refresh no scopes change (grant screen skipped)

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 - not applicable
  • Update any relevant pages in /docs, if necessary
  • For security fixes, the Disclosure Policy must be followed - not applicable

@mkevinosullivan mkevinosullivan changed the title Process embedded parameter to optimize OAuth [WIP] Process embedded parameter to optimize OAuth Aug 3, 2022
@mkevinosullivan mkevinosullivan marked this pull request as ready for review August 6, 2022 19:53
@mkevinosullivan mkevinosullivan changed the title [WIP] Process embedded parameter to optimize OAuth Process embedded parameter to optimize OAuth Aug 6, 2022
paulomarg
paulomarg approved these changes Aug 8, 2022
View reviewed changes
Copy link
Contributor

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

Good test coverage, mostly just nits!

app/controllers/concerns/shopify_app/require_known_shop.rb Outdated Show resolved Hide resolved
app/controllers/shopify_app/callback_controller.rb Outdated Show resolved Hide resolved
lib/shopify_app/controller_concerns/redirect_for_embedded.rb Show resolved Hide resolved
lib/shopify_app/controller_concerns/sanitized_params.rb Show resolved Hide resolved
lib/shopify_app/controller_concerns/sanitized_params.rb Show resolved Hide resolved
test/controllers/callback_controller_test.rb Outdated Show resolved Hide resolved
test/controllers/callback_controller_test.rb Outdated Show resolved Hide resolved
test/controllers/sessions_controller_test.rb Outdated Show resolved Hide resolved
test/controllers/sessions_controller_test.rb Outdated Show resolved Hide resolved
paulomarg
paulomarg approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

Just a nit

test/controllers/sessions_controller_test.rb Outdated Show resolved Hide resolved
paulomarg
paulomarg reviewed Aug 11, 2022
View reviewed changes
shopify_app.gemspec Outdated Show resolved Hide resolved
paulomarg
paulomarg reviewed Aug 15, 2022
View reviewed changes
docs/Upgrading.md Outdated Show resolved Hide resolved
@mkevinosullivan mkevinosullivan force-pushed the kos/optimize_oauth_performance branch 2 times, most recently from d21c46e to a3b31c2 Compare August 15, 2022 19:49
@mkevinosullivan
Process embedded parameter to optimize OAuth
6b9c4d6
@mkevinosullivan
Refactored to redirect to a specified path
ad973fa 
Also added tests cases for sessions and callback controllers.
@mkevinosullivan
Use decode64 instead of strict_decode64
9b5e93d
@mkevinosullivan
Add allow_other_host param to redirect
deb05fd
@mkevinosullivan
Use auth route if embedded url, no embedded param
9838296 
If the embedded_redirect_url is configured but there's no embedded param
received, then go directly to the auth route, rather than via top-level
path.
@mkevinosullivan
Refactor to separate concerns
8c515dd
@mkevinosullivan
Add RedirectForEmbedded to ShopAccessScopesVerification
6203002
@mkevinosullivan
Add call to embedded_app_url in home controller
52c9dc1
@mkevinosullivan
Add path or return_to to redirects
44d14c8
@mkevinosullivan
Update tests to use existing test config
3bc4920
@mkevinosullivan
Add embedded_param method
05138b3
@mkevinosullivan
Add embedded_redirect_url method
d490c17
@mkevinosullivan
Move setup_context to ShopifyAppConfigurer
20683a3
@mkevinosullivan
Refactor use of Context.setup
522e455
@mkevinosullivan
Default scripttags to nil in configurer
0e83355
@mkevinosullivan mkevinosullivan merged commit b382c12 into main Aug 22, 2022
@mkevinosullivan mkevinosullivan deleted the kos/optimize_oauth_performance branch August 22, 2022 15:03
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems August 22, 2022 15:23 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyw8 andyw8 andyw8 left review comments

@paulomarg paulomarg paulomarg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mkevinosullivan @andyw8 @paulomarg