Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Adding AuthScopes value object #110

Merged
merged 1 commit into from
Feb 18, 2021
Merged

Adding AuthScopes value object #110

merged 1 commit into from
Feb 18, 2021

Conversation

paulomarg
Copy link
Contributor

@paulomarg paulomarg commented Feb 12, 2021

WHY are these changes introduced?

We need to add the ability to tell whether an app's scopes have changed to the library. We've recently done that for shopify_api by adding a value object to hold the scopes instead of an array / string.

WHAT is this pull request doing?

Creating a new AuthScopes value object, which abstracts implied scopes and allows to easily compare two sets of scopes.

It also changes the type of Context.SCOPES to be AuthScopes, which enables apps to easily compare if a session has different scopes than the current ones:

if (!Shopify.Context.SCOPES.equals(session.scope)) {
  console.log('Different scopes!!!');
}

cc @rezaansyed and @NabeelAhsen - I basically ripped off the logic from shopify_api here!

Next up:

  • Change koa-shopify-auth's verifyRequest middleware to trigger OAuth if the current session's scopes no longer equal the app's current scopes

@paulomarg paulomarg requested a review from a team as a code owner February 12, 2021 22:01
@paulomarg paulomarg force-pushed the add_scope_value_object branch 2 times, most recently from 411a9ef to 6e89875 Compare February 12, 2021 22:28
@paulomarg paulomarg force-pushed the add_scope_value_object branch from 6e89875 to 81895e4 Compare February 12, 2021 22:32
@paulomarg paulomarg changed the title [WIP] Adding AuthScopes value object Adding AuthScopes value object Feb 17, 2021
expect(scopes.toString()).toEqual('read_products,read_orders,unauthenticated_write_customers');
});

it('trims implied scopes', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about the "implied scopes" bit of this. I don't think this is something I'm familiar with. It looks like if an app has a write scope, we trim the read scope. Is this because having write access also grants read access?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, basically having write_something access also gives you read_something. We're basically omitting the implied scopes from the comparison so that we only return false if the effective permissions are actually different.

Copy link
Contributor

@thecodepixi thecodepixi left a comment

Choose a reason for hiding this comment

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

This looks excellent! I think this takes care of the scope changes really nicely

@paulomarg paulomarg merged commit f8d8339 into main Feb 18, 2021
@paulomarg paulomarg deleted the add_scope_value_object branch February 18, 2021 16:04
@paulomarg paulomarg temporarily deployed to production February 18, 2021 16:24 Inactive
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants