ShopStrategy#update_access_scopes? should only be true when user needs to approve new scopes

[zernie]

Issue summary

Before opening this issue, I have:

• [x] Upgraded to the latest version of the package
  • shopify_app version:
  • Ruby version: 3.2.0
  • Operating system: Ubuntu
• [x] Set log_level: :debug in my configuration, if applicable
• [x] Found a reliable way to reproduce the problem that indicates it's a problem with the package
• [x] Looked for similar issues in this repository
• [x] Checked that this isn't an issue with a Shopify API
  • If it is, please create a post in the Shopify community forums or report it to Shopify Partner Support

---

We are dynamically requesting access scopes based on what features our clients want. This means that config.scope = ''.

When a store requests a certain feature, we prompt him to go through the OAuth flow with the additional scopes. This works fine. However...

Expected behavior

When a user logs out & signs in again, the shop access scopes should not change.

Actual behavior

When a user logs out & signs in again, the shop access scopes are reset to config.scope. The reason for that is this method:

module ShopifyApp
  module AccessScopes
    class ShopStrategy
      class << self
        def update_access_scopes?(shop_domain)
          shop_access_scopes = shop_access_scopes(shop_domain)
          configuration_access_scopes != shop_access_scopes
        end

It should only return true if the shop is missing some scopes, not when it has more that are set in config.scope. I believe UserStrategy is already following this logic - it only asks a user to confirm the new scopes when it's a superset of the existing ones.

A potential fix

module ShopifyApp
  module AccessScopes
    class ShopStrategy
      class << self
        def update_access_scopes?(shop_domain)
          shop_access_scopes = shop_access_scopes(shop_domain)
          (configuration_access_scopes.to_a - shop_access_scopes.to_a).present?
        end
      end
    end
  end
end

[paulomarg]

Hey, thanks for raising this. I think this makes sense, we'll look into it and make sure the suggested fix doesn't carry any risks - I think we might have to replace the token if you remove scopes from an app via the config value, which might not play well with the fix.

One potential workaround for this would be to disable scope checks completely with the reauth_on_access_scope_changes setting, if that works for you. You can always trigger a new authorization request by removing the session from the database when the scopes you'll need change.

If not, you can use ShopSessionStorage instead of ShopSessionStorageWithScopes in your model (or even implement a custom check for your case!).

[zernie]

Hey @paulomarg thanks a lot for the quick response

Hey, thanks for raising this. I think this makes sense, we'll look into it and make sure the suggested fix doesn't carry any risks - I think we might have to replace the token if you remove scopes from an app via the config value, which might not play well with the fix.

I'm not quite sure I get this.

In our case, config.scope never changes.

However, when a certain user needs an additional scope he is prompted to the confirmation screen (/login?...) with the missing scopes. This works great, (it'd be cool if this flow was documented) shop.access_scopes are updated properly and an offline shopify_token is saved. Here's how we do it:

callback_url = ShopifyApp.configuration.login_callback_url.gsub(%r{^/}, '')
        cookie, auth_route = ShopifyAPI::Auth::Oauth.begin_auth(
          shop: current_shop.shopify_domain,
          redirect_path: "/#{callback_url}",
          is_online: false,
          scope_override: <our scopes>
        ).values_at(:cookie, :auth_route)

        cookies.encrypted[cookie.name] = {
          expires: cookie.expires,
          secure: true,
          http_only: true,
          value: cookie.value
        }

        redirect_to auth_route

However, after a re-login (with config.reauth... enabled), shop.access_scopes are reset back to config.scope, which is empty.

also we now need to re-request an online token for the user.

Are we doing this right?:)

[paulomarg]

Hey, thanks for raising this. I think this makes sense, we'll look into it and make sure the suggested fix doesn't carry any risks - I think we might have to replace the token if you remove scopes from an app via the config value, which might not play well with the fix.

I'm not quite sure I get this.

I was mostly talking about other scenarios, where the app uses config.scopes but removes one of those - I think we might run into trouble in that scenario, but it doesn't apply here.

In any case, I think I understand your problem better now - basically, your override is working for the first auth, but it doesn't get re-applied because we default to config.scopes in those cases.

I'm assuming you're already overriding the sessions controller in order to set your overrides? If not, that might be an alternative.

I also noticed that there is currently no way to override the shop scope strategy in the config, other than monkeypatching. One way to solve this would be to override the scope strategy (i.e. write your own version of ShopStrategy) and configure that as an override. That way you'd have full control over how that comparison works.

WDYT?

[zernie]

I'm assuming you're already overriding the sessions controller in order to set your overrides? If not, that might be an alternative.

nope, we haven't considered that. we have a separate controller that redirects user to the /login page when a customer needs new scopes.

I also noticed that there is currently no way to override the shop scope strategy in the config, other than monkeypatching. One way to solve this would be to override the scope strategy (i.e. write your own version of ShopStrategy) and configure that as an override. That way you'd have full control over how that comparison works.

Yep, that'd be great! Currently, we have to resort to monkey-patching.

---

We've also noticed another issue: Sometimes, the app/uninstalled webhook is delayed by a few minutes. So, when the app is quickly reinstalled, we can't reset the Shop#access_scopes in time.

In addition to that, a webhook job can be slow to process. Have you considered using ShopifyAPI::AccessScope.all instead of Shop#access_scopes to check whether access scopes need to be requested? This should fix all potential issues.

Cc @pavel-alexeichik

[paulomarg]

nope, we haven't considered that. we have a separate controller that redirects user to the /login page when a customer needs new scopes.

That's fair - I think for this to work properly with the current code, the right fix would be to replace the sessions controller with one that extends the default, and replace the redirect_to_begin_oauth method to also include your scopes - that way, automatic re-auths will also request the appropriate scopes.

For the future, I feel like we're lacking the ability to customize this flow - maybe a custom_shop_scopes setting that can be a callback would do the trick. That way you wouldn't even need a custom login flow to begin with, you'd just add that config and we'd do the authing for you.

For the sake of transparency, assuming that overriding the sessions controller works, there are other things that might take precedence over this, but I'll add it to our tracking.

Have you considered using ShopifyAPI::AccessScope.all instead of Shop#access_scopes to check whether access scopes need to be requested?

Yes, webhooks can take a little while to be sent. The big downside of getting the scopes from the API is that we basically run this check on every request, which would mean an added round trip to Shopify. That might slow apps down too much, so I'm not sure that's something we want to do.

[zernie]

That's fair - I think for this to work properly with the current code, the right fix would be to replace the sessions controller with one that extends the default, and replace the redirect_to_begin_oauth method to also include your scopes - that way, automatic re-auths will also request the appropriate scopes.

Thanks, I'll look into it.

For the future, I feel like we're lacking the ability to customize this flow - maybe a custom_shop_scopes setting that can be a callback would do the trick. That way you wouldn't even need a custom login flow to begin with, you'd just add that config and we'd do the authing for you.

That'd be perfect!

Yes, webhooks can take a little while to be sent. The big downside of getting the scopes from the API is that we basically run this check on every request, which means an added round trip to Shopify. That might slow apps down too much, so I'm not sure that's something we want to do.

Would it work if that check was only done in SessionsController?

Oh, and one more thing. When we request additional scopes, we request an offline shop token. However, it seems that after reauth an online token is needed to update a User, so we had to add this before_action:

def ensure_has_associated_user
        return if current_shopify_session&.associated_user.present?

        url =  ShopifyApp.configuration.login_url
        query_params = {
          top_level: true,
          shop: current_shopify_session.shop,
          return_to: RedirectSafely.make_safe(session[:return_to] || params[:return_to], nil)
        }
        url = "#{url}?#{query_params.to_query}"

        redirect_to(url)
      end

and this snippet to our ShopUninstallJob:

shop.update!(
          access_scopes: '',
          shopify_token: ""
        )

[paulomarg]

Would it work if that check was only done in SessionsController?

I don't think that would be possible, because we'll usually check the scopes on every request (within the concerns we export) - if we checked only in the sessions controller we'd end up missing checks, so it would behave inconsistently, I believe.

However, it seems that after reauth an online token is needed to update a User

Yes, when using online tokens, you'll need to regenerate them both when the scopes change, so you'll need to go through OAuth twice in that case. In the default callback we'll start the online flow right after the offline one completes, and it will also check the scopes to see if that needs to happen.

This would probably also be affected by the new setting I proposed - when checking the scopes, we'd call the callback here too, so it should still work as expected. In your case now, it'll probably always return true because the scopes will mismatch, so it shouldn't break things.

Hope this helps!