Provide Scopes value object to encapsulate scope operations

[rezaansyed]

This is a follow up on our discussion: Shopify/omniauth-shopify-oauth2#96 (comment)

What does this PR solve?

There are common operations that are used for scopes on OmniAuth and apps consuming the gem. Such operations include serializing, deserializing and normalizing scopes. This PR introduces the Scopes value object that will be used to perform such operations. This helps ensure that apps do not potentially write duplicate code for scopes and delegating this responsibility to the Shopify OmniAuth gem.

Examples of improvements

Simplifying valid scope checks

Before

def valid_scope?(token)
  return false unless token && params[:scope] && token['scope']
  expected_scope = normalized_scopes(params[:scope]).sort
  (expected_scope == token['scope'].split(SCOPE_DELIMITER).sort)
end

def normalized_scopes(scopes)
  scope_list = scopes.to_s.split(SCOPE_DELIMITER).map(&:strip).reject(&:empty?).uniq
  ignore_scopes = scope_list.map { |scope| scope =~ /\A(unauthenticated_)?write_(.*)\z/ && "#{$1}read_#{$2}" }.compact
  scope_list - ignore_scopes
end

After

def valid_scope?(token)
  return false unless token && params[:scope] && token['scope']
  expected_api_access = ShopifyAPI::ApiAccess.new(config[:scope])
  actual_api_access = ShopifyAPI::ApiAccess.new(token['scope'])

  actual_api_access == expected_api_access
end

Delegating check for whether scopes are mismatched or subset of another

def scopes_mismatch?(existing_scopes, requesting_scopes)
  existing_access = ShopifyAPI::ApiAccess.new(existing_scopes)
  requested_access = ShopifyAPI::ApiAccess.new(requesting_scopes)
  
  existing_access == requested_access
end

def scopes_subset?(existing_scopes, requesting_scopes)
  existing_access = ShopifyAPI::ApiAccess.new(existing_scopes)
  requested_access = ShopifyAPI::ApiAccess.new(requesting_scopes)

  requested_access.covers?(existing_access)
end

[NabeelAhsen]

Looks solid!

[paulomarg]

LGTM! Couple of nitpicks only.

[paulomarg]

Nit: should this class be named ApiAccessTest?

[paulomarg]

Should this be a string?

[rezaansyed]

It can either be a string or an array of scopes so that we can support two types of inputs when instantiating ApiAccess.

[paulomarg]

I was just wondering if it made sense for serialized_scopes to be an array - I thought the tests was slightly off in that but it seems like it's just the variable name that is ''''wrong''''. No reason to change it 🙂

[greatestape]

It's sort of hard to picture what the big picture is with this change.

Would it make sense to update the description to show a preview of how we could use this new feature in shopify_app and/or omniauth-shopify-oauth2?

[greatestape]

LGTM

[alexaitken]

Small change to the test relying on the order of the elements from to_a.

[alexaitken]

This assert assumes the order of the elements. ApiAccess operates as a set, the order of the array can not be guaranteed.

There is no built in minitest assert for this, adding .sort to the end of each array will reorder the arrays before checking equality.