-
Notifications
You must be signed in to change notification settings - Fork 678
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
Fixing online token creation #1413
Conversation
3f4c5ea
to
934e29d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎩 👍
I created an app with this template using the CLI, it's now working and no more redirect loops!
|
||
def start_user_token_flow?(shopify_session) | ||
return false unless ShopifyApp::SessionRepository.user_storage.present? | ||
return false if shopify_session.online? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does shopify_session.online?
check whether the response if for an online token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is part of Shopify API, maybe this should be is_online
instead? https://github.com/Shopify/shopify_api/blob/5a4785a40be012f7f3ec93ecb037508efff0c292/lib/shopify_api/auth/session.rb#L47
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe is_online
is defined on shopify-api
. I would keep online?
as I'm getting an undefined error when tophatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I misread it! @paulomarg, do you have additional context for this part? From my understanding, if the app is requesting an online token, then the merchant will be taken through the auth process twice starting with an offline token. If the last token received is an online token, then we don't initiate a redirect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe that is the original behaviour here - if we want online tokens, we go through OAuth twice in a row to get both of them.
@hannachen I noticed we weren't properly cleaning up all of the configuration values when setting up for tests, which led to this inconsistent behaviour. I've now changed the setup config so that it always resets all values (that I could find). I also isolated a bit of code for mocking a session which was what actually broke in the tests as part of my changes. |
fcaf655
to
8f08f20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎩 'd and LGTM!
34909d7
to
5d4c6b9
Compare
What this PR does
We unintentionally dropped some important functionality from this gem as part of our updates of
shopify_api
: the OAuth flow was expected to create both an offline and an online tokens when using user storage, but the updated flow was only doing one of them.This PR walks that back by adding back the behaviour of first getting an offline token, then an online token, then completing the OAuth process, but only in cases where online tokens are required - offline flows remain the same (only a shop is added to the DB).
Reviewer's guide to testing
rails generate shopify_app:user_model
config.user_session_repository = 'User'
in shopify_app.rbshopify app serve
=> go through loginThings to focus on
Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.md
if the changes would impact users