[Bug fix] Redirect EnsureAuthenticatedLinks to login page if shop domain not found

[NabeelAhsen]

Fixes the issue brought up by a partner #1118 (comment)

In a world without cookies, an embedded app needs to handle attempted access to protected resources both in an embedded and non-embedded state. This PR fixes the scenario where ShopifyApp::LoginProtection::ShopifyDomainNotFound is raised when:

• The app is in a non-embedded state
• No shop parameter is passed in
• The app is unauthenticated
• The browser does not support 3p cookies (and subsequently does not have a 1p cookie saved on its domain with a sample shop domain)

In this scenario, rather than raise an exception, we should redirect to the app's login_url.

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.

[rezaansyed]

LGTM!

[thecodepixi]

This makes sense to me, but I'm curious about why we don't want to raise anything here? Would we want to communicate to the app user that some information was missing or that something went wrong? Or is the scenario simply a "you need to actually log in to do that" in which case landing on the login page is enough?

[NabeelAhsen]

This makes sense to me, but I'm curious about why we don't want to raise anything here? Would we want to communicate to the app user that some information was missing or that something went wrong? Or is the scenario simply a "you need to actually log in to do that" in which case landing on the login page is enough?

I think the message that is being conveyed here is:

If the app can't determine anything about the current shop in context, then the app is most likely not being opened in a shop's Admin. In this case, the login flow is necessary to get the app into a good state.

With the current implementation, app developers see a nasty rails error thrown. The likely move in this scenario would be to (almost always?) go login and get the app into an embedded state.

The LoginProtection concern previously took care of this at the cost of depending on cookies. This is behaviour that we'd ideally like to isolate and silo in the future as we move away from cookie dependency.

Edit: I'm still on the fence on whether hiding this error message is the right call or not, but rescuing it here is more consistent with the experience users had in the world of cookies.

[paulomarg]

Edit: I'm still on the fence whether hiding this error message is the right call or not, but rescuing it here is more consistent with the experience users had in the world of cookies.

Those are all good points. If we're not sure if this is the right behaviour, would it make sense to log this, so we have some means of keeping track of it?

[NabeelAhsen]

Those are all good points. If we're not sure if this is the right behaviour, would it make sense to log this, so we have some means of keeping track of it?

The behaviour I'm proposing in this PR is actually mirrored here under a slightly different context:

shopify_app/app/controllers/concerns/shopify_app/require_known_shop.rb

Lines 19 to 21 in 5af43ef

	def check_shop_domain
	redirect_to(ShopifyApp.configuration.login_url) unless current_shopify_domain
	end

There is work underway that aims to add deliberate observability (and logging) to this gem. I'm tempted to defer the decision to log anything here until we can inventory the circumstances for redirects; this includes stale sessions, cookie dependencies, and expired tokens. What are your thoughts on this?

[paulomarg]

That sounds sensible to me. I would advocate for adding the logging ahead of the bigger refactoring only if this is a risky scenario where we could break apps. Since this is a very specific case and every condition points to there not being a session it is probably safe to defer it.

That being said, I would definitely flag this as a case to consider. IMHO no error should go away silently, but this has a user-facing result (of redirecting to login) and thus is detectable (and reportable) by end users if they see it happening.

[NabeelAhsen]

That being said, I would definitely flag this as a case to consider. IMHO no error should go away silently, but this has a user-facing result (of redirecting to login) and thus is detectable (and reportable) by end users if they see it happening.

On second thought, you've convinced me that it's probably worth adding logging now rather than deferring it. It would help partners navigate this behaviour if it's something they did not expect by switching to session tokens.

Thank you for the thorough review!

[paulomarg]

LGTM! Strangely I convinced you to do this by trying to make the point not to 😄

In any case, I think this kind of logging is always valuable when things go wrong, so I'm all for it.