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.

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

Allow custom scopes during the auth process #1023

Merged
merged 1 commit into from
Oct 19, 2022

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Sep 28, 2022

Description

First party apps such as https://shopify-graphiql-app.shopifycloud.com have scopes which are set during installation, rather than being preconfigured.

There is a corresponding update to shopify_app: Shopify/shopify_app#1540

How has this been tested?

Tested as part of the upgrade of https://shopify-graphiql-app.shopifycloud.com

Checklist:

  • My commit message follow the pattern described in here
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the project documentation N/A
  • I have added a changelog line.

@andyw8 andyw8 force-pushed the andyw8/allow-begin_auth-scopes-override branch from 9640704 to 511daf9 Compare October 18, 2022 13:38
@andyw8 andyw8 changed the title WIP: Allow begin_auth scopes override Allow custom scopes during the auth process Oct 18, 2022
@andyw8 andyw8 force-pushed the andyw8/allow-begin_auth-scopes-override branch from bdf9b6c to 45c378b Compare October 18, 2022 20:37
@@ -16,9 +16,10 @@ class << self
shop: String,
redirect_path: String,
is_online: T.nilable(T::Boolean),
scope: ShopifyAPI::Auth::AuthScopes,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should really be called scopes but we're already using scope to refer to the collection in multiple places, so I've followed that pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's true - I think this is a consequence of the authorize request and token response actually using scope: https://shopify.dev/apps/auth/oauth/getting-started#online-access-mode

Should we call this scope_override so that it's more obvious that this is not a required argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 updated

@andyw8 andyw8 force-pushed the andyw8/allow-begin_auth-scopes-override branch from 45c378b to c51c59d Compare October 18, 2022 20:40
@andyw8 andyw8 marked this pull request as ready for review October 18, 2022 20:40
@andyw8 andyw8 requested a review from a team as a code owner October 18, 2022 20:40
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 non-blocking comments.

@@ -16,9 +16,10 @@ class << self
shop: String,
redirect_path: String,
is_online: T.nilable(T::Boolean),
scope: ShopifyAPI::Auth::AuthScopes,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's true - I think this is a consequence of the authorize request and token response actually using scope: https://shopify.dev/apps/auth/oauth/getting-started#online-access-mode

Should we call this scope_override so that it's more obvious that this is not a required argument?

@@ -85,6 +85,13 @@ def test_begin_auth_online
verify_oauth_begin(auth_route: result[:auth_route], cookie: result[:cookie], is_online: true)
end

def test_custom_scope
result = ShopifyAPI::Auth::Oauth.begin_auth(shop: @shop, redirect_path: "/redirect",
scope: ShopifyAPI::Auth::AuthScopes.new("read_orders,write_products"))
Copy link
Contributor

Choose a reason for hiding this comment

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

The AuthScopes object is capable of taking in a string / string[] and converting it, we could consider allowing any of the 3 types as the argument to make it easier on callers.

@andyw8 andyw8 force-pushed the andyw8/allow-begin_auth-scopes-override branch 3 times, most recently from 77f6808 to 475f63d Compare October 19, 2022 20:07
@andyw8 andyw8 force-pushed the andyw8/allow-begin_auth-scopes-override branch from 475f63d to a631944 Compare October 19, 2022 20:18
@andyw8 andyw8 merged commit c8a8815 into main Oct 19, 2022
@andyw8 andyw8 deleted the andyw8/allow-begin_auth-scopes-override branch October 19, 2022 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants