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

SessionRepository#delete_session shouldn't use actual models when destroying records #1723

Closed
zzooeeyy opened this issue Sep 5, 2023 · 0 comments · Fixed by #1773
Closed
Assignees

Comments

@zzooeeyy
Copy link
Contributor

zzooeeyy commented Sep 5, 2023

Issue summary

SessionRepository#delete_session is using the actual model classes (User, Shop) when destroying records.
We shouldn't be doing this since other apps might not use the same models.

In code:

 def delete_session(id)
  match = id.match(/^offline_(.*)/)

  record = if match
    domain = match[1]
    ShopifyApp::Logger.debug("Destroying session by domain - domain: #{domain}")
    Shop.find_by(shopify_domain: match[1]) ######### <------------ Problematic
  else
    shopify_user_id = id.split("_").last
    ShopifyApp::Logger.debug("Destroying session by user - user_id: #{shopify_user_id}")
    User.find_by(shopify_user_id: shopify_user_id)  ######### <------------ Problematic
  end

  record.destroy

  true
end
  • shopify_api version: latest
  • shopify_app version: latest (21.6.0)
  • Ruby version: any
  • Operating system: any

Expected behavior

Proxy through SessionRepository.user_storage or SessionRepository.shop_storage similar to load_session and store_session.

Actual behavior

We should not be using the Shop or User models directly since it might not exist on other applications.

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 a pull request may close this issue.

2 participants