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

Don't redirect on mismatching scopes if configuration is disabled #1639

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

minichate
Copy link
Contributor

@minichate minichate commented Jan 11, 2023

What this PR does

This skips the redirect-to-reauthenticate block in LoginProtection

We had an incident earlier this week where we were doing a scope upgrade and despite us disabling reauth_on_access_scope_changes ahead of time.

The scopes were updated/backfilled on the Shopify Core side, but since the cached scopes in user and shop models were a subset of the actually-approved scopes in Core requests were rejected with a 401 response without even sending them to Core.

This was surprising behaviour to us since we'd disabled the feature intentionally ahead of time.

Reviewer's guide to testing

  1. Have an existing set of scopes cached on the user and shop models
  2. Add a new scope c (ie: in config.scope)
  3. Set reauth_on_access_scope_changes = false in your app's shopify_app initializer
  4. Try and use the app; you'll get a 401 response from XHR requests, and a redirect for regular document requests.

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.

@minichate minichate requested a review from jeremywrowe January 11, 2023 15:44
@minichate minichate self-assigned this Jan 11, 2023
@minichate minichate force-pushed the dont-redirect-if-not-reauth branch 3 times, most recently from 4833a87 to 2b098f8 Compare January 11, 2023 16:21
@jeremywrowe
Copy link
Contributor

I approved, the code looks good. In addition to the change there needs to be updates to documentation and the changelog.

@minichate minichate force-pushed the dont-redirect-if-not-reauth branch from 2b098f8 to 5bc848f Compare February 21, 2023 15:00
@minichate minichate force-pushed the dont-redirect-if-not-reauth branch from 5bc848f to 22500da Compare February 21, 2023 15:06
@minichate minichate merged commit 4bec615 into main Feb 21, 2023
@minichate minichate deleted the dont-redirect-if-not-reauth branch February 21, 2023 15:27
@minichate minichate mentioned this pull request Feb 21, 2023
4 tasks
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems February 21, 2023 15:47 Inactive
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.

2 participants