-
Notifications
You must be signed in to change notification settings - Fork 684
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
Move covered scopes check into strategy #1600
Conversation
@@ -12,6 +12,11 @@ def update_access_scopes?(user_id: nil, shopify_user_id: nil) | |||
"#update_access_scopes? requires user_id or shopify_user_id parameter inputs") | |||
end | |||
|
|||
def covered_scopes?(current_shopify_session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe covers_scopes?
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@paulomarg @nelsonwittwer sharing as a draft, any thoughts so far? |
unless current_shopify_session.scope.to_a.empty? || | ||
current_shopify_session.scope.covers?(ShopifyAPI::Context.scope) | ||
|
||
unless ShopifyApp.configuration.user_access_scopes_strategy.covered_scopes?(current_shopify_session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should both clauses of the unless
be in the strategy? Or only the second? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's really possible for a session to have empty scopes, unless there's a bug in the storage code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect so. I don't want to change the behaviour though so I'll leave it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me so far!
@@ -12,6 +12,11 @@ def update_access_scopes?(user_id: nil, shopify_user_id: nil) | |||
"#update_access_scopes? requires user_id or shopify_user_id parameter inputs") | |||
end | |||
|
|||
def covered_scopes?(current_shopify_session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that better.
unless current_shopify_session.scope.to_a.empty? || | ||
current_shopify_session.scope.covers?(ShopifyAPI::Context.scope) | ||
|
||
unless ShopifyApp.configuration.user_access_scopes_strategy.covered_scopes?(current_shopify_session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's really possible for a session to have empty scopes, unless there's a bug in the storage code.
@@ -12,6 +12,11 @@ def update_access_scopes?(user_id: nil, shopify_user_id: nil) | |||
"#update_access_scopes? requires user_id or shopify_user_id parameter inputs") | |||
end | |||
|
|||
def covered_scopes?(current_shopify_session) | |||
# NOTE: this not Ruby's `covers?` method, it is defined in ShopifyAPI::Auth::AuthScopes | |||
current_shopify_session.scope.to_a.empty? || current_shopify_session.scope.covers?(ShopifyAPI::Context.scope) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to illustrate what covers?
does:
abc = ShopifyAPI::Auth::AuthScopes.new(['A', 'B', 'C'])
ab = ShopifyAPI::Auth::AuthScopes.new(['A', 'B'])
abc.covers?(ab) => true
ab.covers?(abc) => false
abc.covers?(abc)= > true
See https://github.com/Shopify/shopify-api-ruby/blob/main/test/auth/auth_scopes_test.rb for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a non-blocking comment.
unless current_shopify_session.scope.to_a.empty? || | ||
current_shopify_session.scope.covers?(ShopifyAPI::Context.scope) | ||
|
||
unless ShopifyApp.configuration.user_access_scopes_strategy.covers_scopes?(current_shopify_session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember we saw another place where we did a similar check (callback controller?), we should make sure to use the same check in both cases for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 This seem to be the only use of covers?
...
* Move covered scopes check into strategy * Rename method * Update CHANGELOG Co-authored-by: Andy Waite <andyw8@users.noreply.github.com>
What this PR does
There are two places that we check the user's scopes when determining if a re-auth is needed:
update_access_scopes?
in the access scopes user strategy.LoginProtection#activate_shopify_session
In #1599 we added an option to set the user strategy. But this has a gap, since only one of the above listed places is within the strategy. To have proper control over these checks, we need both of them to be in the strategy.
This PR aims to not alter any existing behaviour, it only moves code around.
Reviewer's guide to testing
Things to focus on
Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.md
if the changes would impact usersREADME.md
, if appropriate./docs
, if necessary