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.

Sign up for GitHub

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

Jump to bottom

Validate store sessions #1612

Merged
merged 8 commits into from
Dec 14, 2022
Merged

Validate store sessions #1612

merged 8 commits into from
Dec 14, 2022

Conversation

nelsonwittwer
Copy link
Contributor

@nelsonwittwer nelsonwittwer commented Dec 13, 2022 •
edited
Loading

What this PR does

When a store uninstalls an app we are currently dependent upon the uninstall webhook to tell our apps that a store should no longer be able to access the API / app resources. This leads to apps getting rejected using our templates as reported in #1591 .

This PR will do a lightweight API call after it has found the shop session in storage to ensure it is still valid. If it is not, it will redirect to the embedded login url.

Reviewer's guide to testing

My 🎩 testing looked like this:

  1. Create new app with CLI
  2. Point to local version of this gem within the generated app
  3. Install app in test store
  4. Uninstall app in admin UI
  5. Visit the app directly (not within embedded admin) http://localhost:62121/?shop=test-nelson27.myshopify.com&host=dGVzdC1uZWxzb24yNy5teXNob3BpZnkuY29tL2FkbWlu
  6. Verify that it redirected me to the install page within admin

Checklist

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.

paulomarg
paulomarg reviewed Dec 13, 2022
View reviewed changes
app/controllers/concerns/shopify_app/ensure_installed.rb Outdated
client = ShopifyAPI::Clients::Rest::Admin.new(session: installed_shop_session)
client.get(path: "shop")
rescue ShopifyAPI::Errors::HttpResponseError => error
redirect_for_embedded if error.code == 401 # unauthorized due to uninstall
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be a redirected_for_embedded situation? I think we would call this when NOT embedded, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ya, that's not clear because the conditional for this method is listed in the before_action declaration. I'll fix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, we should only be doing this session sanity check if it is embedded, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should redirect to OAuth, no?

Also the comment here might be misleading - there could be a variety of reasons the token is no longer valid and not just because of uninstall (keys can be revoked, token could be expired, etc)

Copy link
Contributor Author

@nelsonwittwer nelsonwittwer Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should redirect to oauth, which we sadly call login_url for reasons 🤦

@wisniewski94 wisniewski94 mentioned this pull request Dec 13, 2022
Closed
@nelsonwittwer nelsonwittwer changed the title WIP - Validate store sessions Validate store sessions Dec 14, 2022
paulomarg
paulomarg approved these changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only had non-blocking comments!

app/controllers/concerns/shopify_app/ensure_installed.rb

client = ShopifyAPI::Clients::Rest::Admin.new(session: installed_shop_session)
client.get(path: "shop")
rescue ShopifyAPI::Errors::HttpResponseError => error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we re-throw if the error isn't a 401 here? Unlikely, but may happen 🤷

app/controllers/concerns/shopify_app/ensure_installed.rb
@@ -58,5 +63,14 @@ def shop_login

url.to_s
end

def validate_non_embedded_session
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think calling this validate_session_before_embedding would make it easier to understand this method?

test/controllers/concerns/ensure_installed_test.rb Outdated
@@ -61,6 +66,36 @@ def index
assert_response :ok
end

test "redirects to login_url (oauth path) to reinstall the app if the store's session token is no longer valid" do
ShopifyApp.configuration.stubs(:embedded_app?).returns(true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can actually change the configuration value as part of the tests, I'm pretty sure we reset them for each test.

test/controllers/concerns/ensure_installed_test.rb Outdated
shopify_domain = "shop1.myshopify.com"
get :index, params: { shop: shopify_domain }

assert_response :redirect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we assert that the location we go to is /login?

@nelsonwittwer nelsonwittwer merged commit c3e89a9 into main Dec 14, 2022
@nelsonwittwer nelsonwittwer deleted the nelsonwittwer/validate_session branch December 14, 2022 23:28
@uurcank
Copy link
Contributor

uurcank commented Dec 20, 2022

@nelsonwittwer @paulomarg this change checks whether the token is valid but callback_controller does not update the offline token. From what I see if store_session_storage is not empty, same values stay and database not updated with new token value.

Could you please confirm whether this is the case?

fabriazza pushed a commit to fabriazza/shopify_app that referenced this pull request Feb 1, 2023
@nelsonwittwer @fabriazza
Validate store sessions (Shopify#1612)
f1eeacc 
* Check shop offline session is still valid when embedded

* only redirect on 401 and use better naming

* use guard clause instead of conditional filter

* redirect to shop_login if token failed

* update tests to match expected behavior

* Changelog updates

* raise error if not 401

* better redirect test
@netwire88
Copy link

netwire88 commented Feb 17, 2023 •
edited
Loading

Will all the documentation that refers to RequireKnownShop need to be changed to EnsureInstalled?

Also, does EnsureInstalled work with EmbeddedApp and ShopAccessScopesVerification in an unauthenticated controller? It seems that ShopAccessScopesVerification creates an infinite loop even after reviewing https://github.com/Shopify/shopify_app/blob/main/docs/Troubleshooting.md#im-stuck-in-a-redirect-loop-after-oauth

  include ShopifyApp::EmbeddedApp
  include ShopifyApp::EnsureInstalled
  include ShopifyApp::ShopAccessScopesVerification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teddyhwang teddyhwang teddyhwang left review comments

@paulomarg paulomarg paulomarg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nelsonwittwer @uurcank @netwire88 @teddyhwang @paulomarg