Fix omniauth strategy not being set correctly for apps using session tokens

[rezaansyed]

What is the issue?

Currently, new apps using shopify_app have session tokens enabled and cookies based auth disabled by default. The problem is that even at the top level actions where cookies are accessible and appropriate to use, LoginProtection gates it's usage in user_session_by_cookie and shop_session_by_cookie using the allow_cookie_authentication setting. This has led to calls for the user_session and shop_session resulting in nil, leading the app to falsely believe that it doesn't have an offline and/or online token.

OAuth redirect loops for offline tokens

1. [/login] Initiate OAuth to fetch the offline/shop access token
2. Goes through OAuth process
3. [/callback] Store the offline access token
4. [/callback] Respond with redirect to login to fetch online token
5. 🔥 [/login] Fails to set session[:user_tokens] to true as shop_session check returns nil
6. OmniAuth falsely assumes app requires an offline token and starts process to fetch one
7. Repeat steps 1-6 until Shopify stops looping process

OAuth redirect loops for online tokens

1. [/login] Initiate OAuth to fetch the online/user access token
2. Goes through OAuth process
3. [/callback] Store the online access token
4. [/callback] Checks to see if it needs to start online token flow
5. 🔥 [/callback] user_session results in nil
6. [/callback] Respond with redirect to login to fetch online token
7. Repeat steps 1-6 until Shopify stops looping process

What is the fix?

SessionsController and CallbackController can both operate from the top-level. Use of session cookies are justified in these scenarios.

OAuth redirect loops for offline tokens

Override the shop_session_by_cookie method in SessionsController to ensure we are still using session cookies as before on the top level.

OAuth redirect loops for online tokens

Override the user_session_by_cookie method in CallbackController to ensure we are still using session cookies as before on the top level.

🎩

Before

Screen.Recording.2021-01-26.at.4.30.31.PM.mov

After

Screen.Recording.2021-01-26.at.4.22.26.PM.mov

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.

[NabeelAhsen]

I'm not a fan of the overrides here, but let's mark follow-up work to see if we can refactor the *_session_by_cookie methods to behave differently under different contexts

[rezaansyed]

I'm not a fan of the overrides here, but let's mark follow-up work to see if we can refactor the *_session_by_cookie methods to behave differently under different contexts

This is to limit the blast radius of our changes. SessionController and CallbackController are both controllers that operate on the top level and are justified in using session cookies. We need to look further into splitting up components whose lifecycles are in an embedded context from those with lifecycles at the top level.

[greatestape]

LGTM

[greatestape]

LGTM