Add session token concern #986
Conversation
| end | ||
|
|
||
| def check_shop_known | ||
| @shop = Shop.find_by(shopify_domain: @shopify_domain) |
There was a problem hiding this comment.
I think you should be accessing this data via SessionRepository.retrieve_shop_session_by_domain. How the fetch logic is implemented is a detail that's hidden behind the interface.
| # frozen_string_literal: true | ||
|
|
||
| module ShopifyApp | ||
| module DomainProtection |
There was a problem hiding this comment.
Just want to call attention to the name for other reviewers. We wanted to name this something that indicates its responsibility and reflects that it would be used instead of LoginProtection. Does this name do that?
There was a problem hiding this comment.
Maybe RequireKnownShop or something would be more clear what it does?
|
|
||
| def check_shop_known | ||
| @shop = Shop.find_by(shopify_domain: @shopify_domain) | ||
| redirect_to("#{ShopifyApp.configuration.login_url}?shop=#{@shopify_domain}") unless @shop |
There was a problem hiding this comment.
Should we add a return_to parameter so that the redirect returns us back to the page it was last at and not always the homepage? Email folks added a PR fix for this yesterday: https://github.com/Shopify/email/pull/4746
There was a problem hiding this comment.
@rezaansyed I think if the shop doesn't exist we'd want them to go through the login flow again, right?
Email does that because it is trying to masquerade as being part of Shopify and not an app so they redirect to the marketing section.
There was a problem hiding this comment.
I thought the return_to would be the path the user gets taken to within the app iframe after the login flow finishes.
There was a problem hiding this comment.
Oh. Okay, I'll make the changes.
theundeadmonk
force-pushed
the
add-session-token-concern
branch
2 times, most recently
from
45f987c
to
89b9048
Compare
| url.query = URI.encode_www_form( | ||
| shop: params[:shop], | ||
| return_to: request.fullpath, | ||
| ) |
| redirect_url.query = URI.encode_www_form( | ||
| shop: shopify_domain, | ||
| return_to: request.fullpath, | ||
| ) |
theundeadmonk
force-pushed
the
add-session-token-concern
branch
from
89b9048
to
47fe094
Compare
theundeadmonk
force-pushed
the
add-session-token-concern
branch
from
47fe094
to
5e7f336
Compare
| end | ||
|
|
||
| def check_shop_known | ||
| @shop = SessionRepository.retrieve_shop_session_by_shopify_domain(current_shopify_domain) |
There was a problem hiding this comment.
This looks good now!
retrieve_shop_session_by_domain ~> retrieve_shop_session_by_shopify_domain
theundeadmonk
force-pushed
the
add-session-token-concern
branch
from
5e7f336
to
7f0f37f
Compare
|
🎩 on a test app |
theundeadmonk
force-pushed
the
add-session-token-concern
branch
from
7f0f37f
to
e10685f
Compare
theundeadmonk
force-pushed
the
add-session-token-concern
branch
from
e10685f
to
766b3a7
Compare
This PR adds a concern that can be used in an unauthenticated controller.
It does the following
Both the before actions redirect to login in case of failure