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

Make scopes optional for mangaged install token exchange apps #1333

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lizkenyon
Copy link
Contributor

@lizkenyon lizkenyon commented Apr 5, 2024

WHY are these changes introduced?

  • Currently when an App is using managed install, and the scopes in their config object do not match the scopes they have set up in their app toml, it can send the app into an infinite redirect loop.
  • Managed install replaces the functionality where we need to check if the sessions scopes match the scopes in the config.

Assumptions

  • Apps that are using managed install and token exchange, do not need to use the SCOPES environment variable, or the scopes config setting, Shopify will manage redirecting to the authorization page if necessary.

WHAT is this pull request doing?

  • This PR makes the scopes field on the API config object optional.
  • This update the isActive method on the Session object to ignore scope differences if passing in a falsy value.
  • Apps that are not using the new auth strategy do require scopes, so if we start the oauth flow, and scopes are not defined we will throw an error.

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have used yarn changeset to create a draft changelog entry (do NOT update the CHANGELOG.md file manually)
  • I have added/updated tests for this change
  • I have documented new APIs/updated the documentation for modified APIs (for public APIs)

alvaro-shopify
alvaro-shopify previously approved these changes Apr 9, 2024
Comment on lines +72 to +75
throwIfScopesUndefined(
config.scopes,
'Apps that use OAuth must define the required scopes in the config',
);

Choose a reason for hiding this comment

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

Suggested change
throwIfScopesUndefined(
config.scopes,
'Apps that use OAuth must define the required scopes in the config',
);
if (!config.scopes) {
throw new ShopifyErrors.MissingRequiredArgument('Apps that use OAuth must define the required scopes in the config',);
}

I'd rather check the config.scopes nullability here (the throwIfScopesUndefined is not used elsewhere) to avoid using config.scopes!.toString() and rely on TS type checking

*/
public isActive(scopes: AuthScopes | string | string[]): boolean {
const usingScopes =

Choose a reason for hiding this comment

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

I think that you need to add an additional check if scopes is instance of AuthScopes

scopes instanceofAuthScopes ? scopes.toArray().length > 0 : scopes !== '' && typeof scopes !== 'undefined' && scopes !== null

you could extract it to a different method

@alvaro-shopify alvaro-shopify dismissed their stale review April 9, 2024 11:10

I approved it by mistake 😅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants