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

Store access scopes after successful OAuth #1192

Merged
merged 1 commit into from
Feb 25, 2021
Merged

Store access scopes after successful OAuth #1192

merged 1 commit into from
Feb 25, 2021

Conversation

rezaansyed
Copy link
Contributor

@rezaansyed rezaansyed commented Feb 22, 2021 •
edited
Loading

Problem

Embedded apps using session tokens do not automatically handle changes in access scopes for offline and online tokens. In order to handle changes to access scopes requested, we need to store the access scopes after successfully completing OAuth flows.

What does this PR do?

On successful completion of OAuth, apps will now store the access scopes of an access token in ShopifyApp::ShopSessionStorageWithScopes and ShopifyApp::UserSessionStorageWithScopes.

🎩 Tophatting

Go through OAuth for offline/shop tokens

  • Appropriate Shop record should have access_scopes with the expected values

Go through OAuth for online/user tokens

  • Appropriate User record should have access_scopes with the expected values

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 rezaansyed requested a review from a team as a code owner February 22, 2021 23:09
@rezaansyed rezaansyed self-assigned this Feb 22, 2021
@rezaansyed
Copy link
Contributor Author

Tophat of app with offline and online tokens:

store-access-scopes-after-oauth.mov

@rezaansyed
Copy link
Contributor Author

Tophatting for backwards compatibility where existing apps don't have access tokens defined on the Shop or User model. There should not be any access scopes stored.

backwards-compatibility.mov

@rezaansyed rezaansyed force-pushed the store-and-return-access-scopes-in-session-store branch from 21a5714 to 8b3cef4 Compare February 23, 2021 00:02
NabeelAhsen
NabeelAhsen approved these changes Feb 23, 2021
View reviewed changes
Copy link
Contributor

@NabeelAhsen NabeelAhsen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Non-blocking comments, code looks good!

Will test these changes locally again sometime today.

@rezaansyed rezaansyed force-pushed the store-and-return-access-scopes-in-session-store branch 2 times, most recently from 6cc34f6 to 9fad863 Compare February 23, 2021 14:47
@NabeelAhsen NabeelAhsen mentioned this pull request Feb 23, 2021
Merged
@rezaansyed rezaansyed force-pushed the store-and-return-access-scopes-in-session-store branch from b458acb to 3f3395c Compare February 24, 2021 21:35
@rezaansyed rezaansyed requested a review from ragalie February 24, 2021 21:36
@NabeelAhsen NabeelAhsen self-requested a review February 24, 2021 22:32
ragalie
ragalie approved these changes Feb 25, 2021
View reviewed changes
Copy link
Contributor

@ragalie ragalie left a comment

Choose a reason for hiding this comment

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

LGTM, comment is not a blocker

paulomarg
paulomarg approved these changes Feb 25, 2021
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

NabeelAhsen
NabeelAhsen approved these changes Feb 25, 2021
View reviewed changes
Copy link
Contributor

@NabeelAhsen NabeelAhsen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

We should roadmap future work to only support the new SessionStorage interface before a major release.

@rezaansyed rezaansyed force-pushed the store-and-return-access-scopes-in-session-store branch from 3f3395c to 01893b1 Compare February 25, 2021 14:56
@rezaansyed rezaansyed force-pushed the store-and-return-access-scopes-in-session-store branch from 01893b1 to 8babf78 Compare February 25, 2021 14:58
@rezaansyed rezaansyed merged commit 30bb024 into master Feb 25, 2021
@rezaansyed rezaansyed deleted the store-and-return-access-scopes-in-session-store branch February 25, 2021 15:57
@rezaansyed rezaansyed mentioned this pull request Mar 4, 2021
Merged
4 tasks
@rezaansyed rezaansyed temporarily deployed to rubygems March 5, 2021 15:45 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ragalie ragalie ragalie approved these changes

@NabeelAhsen NabeelAhsen NabeelAhsen approved these changes

@paulomarg paulomarg paulomarg approved these changes

@greatestape greatestape Awaiting requested review from greatestape

Assignees

@rezaansyed rezaansyed

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rezaansyed @ragalie @NabeelAhsen @paulomarg