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

Set CurrentTenant to nil when gem is present #1898

Closed
wants to merge 1 commit into from

Conversation

rachel-carvalho
Copy link
Contributor

What this PR does

This makes sure the CurrentTenant is set to nil if gem is present whenever ShopifyApp ends the request in a before action, which happens:

  • When there's no current session when using auth code flow
  • When there's no id_token or if it's invalid when using token exchange

Reviewer's guide to testing

Steps to reproduce the error:

  1. create a new app with the CLI
  2. make sure it has write_products scope and shopify app deploy
  3. make sure new_embedded_auth_strategy is false
  4. add the line to set the current tenant to HomeController#index, ProductsController#count and ProductsController#create
  5. add the security gem
  6. bundle and start the server
  7. install the app
  8. delete the row created in the shops table
  9. click on Populate 5 products
  10. 👀 see the current tenant exception in the server logs 💥

Seeing the fix

  1. point shopify_app in the Gemfile to this branch with gem "shopify_app", github: "Shopify/shopify_app", ref: "fix_current_tenant_error"
  2. bundle and restart the server
  3. open the app again
  4. delete the row created by install in the shops table
  5. click on Populate 5 products
  6. 👀 see how it correctly redirects to get a new access token ✨

Things to focus on

I couldn't find a nicer way to test the path of defined?(ClassName), I'd appreciate if anyone has better suggestions.

Anything else I'm missing?

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.

@rachel-carvalho rachel-carvalho requested a review from a team as a code owner August 20, 2024 21:14

def with_shopify_security_base_current_tenant
shopify_security_base_module = Module.new
ShopifyApp::LoginProtection.const_set("ShopifySecurityBase", shopify_security_base_module)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a nicer way to stub the existence of a class than using const_set then remove_const (which for some reason is private even though const_set isn't). Open to other ideas if anyone has them!

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.

1 participant