Refactor oauth strategy

[rezaansyed]

WHY are these changes introduced?

Part of https://github.com/Shopify/develop-app-access/issues/134

This change enables us to add token exchange in a following PR. This does not change any behaviour to the current OAuth authorization code flow. It is simply a refactor of the AuthStrategy class.

WHAT is this pull request doing?

• Extracts auth code flow logic into AuthCodeFlowStrategy class, which is injected into the AuthStrategy class.
  • This enables us to add the eventual Token Exchange strategy class with minimum complexity and footprint.

Type of change

• Patch: Bug (non-breaking change which fixes an issue)
• Minor: New feature (non-breaking change which adds functionality) This is a refactor. No functionality added or removed.
• 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 files manually)
• I have added/updated tests for this change
• I have documented new APIs/updated the documentation for modified APIs (for public APIs)

[rezaansyed]

We can remove this test, if we're okay with keeping auth-callback-path.test.ts and auth-path.test.ts

[byrichardpowell]

As messaged on Slack i'd like to step through through these changes together on tuple to understand them a bit more. Without that high speed back n forth it's quite a lot to understand and quite hard to do the review justice

Adding a mini review now, but let's chat before actioning anything here

[byrichardpowell]

Since partners won't care about this, I'm not sure if this needs a release note? Thoughts @paulomarg

[byrichardpowell]

Both the session token and the URL params expire and can only be generated using the API secret. Given that, does token exchange need to validateURL Params?

[zzooeeyy]

I feel like this is path specific logic is really scattered and difficult to see all the routes we handle, and this url parsing is repeated too many times throughout this common handleRoutes logic and the strategy specific handleRoutes logic.

Just throwing some ideas:

1. Would it benefit from using a switch statement so that we can see from a glance all the routes we handle

private async handleRoutes(request: Request) {
  const {config, logger, api} = this;
  const url = new URL(request.url); 

  switch(url.pathname) {
    case config.auth.patchSessionTokenPath:
       logger.debug('Rendering bounce page');
      throw renderAppBridge({config, logger, api}, request); 
    case config.auth.exitIframePath:
      const destination = url.searchParams.get('exitIframe')!;
      logger.debug('Rendering exit iframe page', {destination});
      throw renderAppBridge({config, logger, api}, request, {url: destination});
   .... 
     etc
  } 
}

2. If we do what Richard suggested here

the handleRoutes logic can be in the common class so that:

// ******* Common Class
private async handleRoutes(request: Request) {
  const {config, logger, api} = this;
  const url = new URL(request.url); 

  switch(url.pathname) {
    case config.auth.patchSessionTokenPath:
       logger.debug('Rendering bounce page');
      throw renderAppBridge({config, logger, api}, request); 
    case config.auth.exitIframePath:
      const destination = url.searchParams.get('exitIframe')!;
      logger.debug('Rendering exit iframe page', {destination});
      throw renderAppBridge({config, logger, api}, request, {url: destination});
   .... 
     etc
  } 
}

// ********** Auth strategy specific class:
private async handleRoutes(request: Request) {
  const {config, logger, api} = this;
  const url = new URL(request.url); 

  switch(url.pathname) {
    case config.auth.path:
       throw await this.handleAuthBeginRequest(request, shop)
    case config.auth.callbackPath:
       throw await this.handleAuthCallbackRequest(request, shop)
    default  :
       // Call common class's method `handleRoutes` to handle "patchSessionTokenPath" & "exitIframePath" by default
   .... 
     etc
  } 
}

[owlyowl]

This log displays for people using online tokens it's a bit of a misnomer

[paulomarg]

This should happen in both cases - using online tokens causes OAuth to create both offline and online, and we use the presence of the offline token as indication that the app was installed (because it doesn't expire).

[paulomarg]

LGTM, I had a bunch of nits but none were blocking.

[paulomarg]

This should happen in both cases - using online tokens causes OAuth to create both offline and online, and we use the presence of the offline token as indication that the app was installed (because it doesn't expire).

[paulomarg]

I like having these in the same file, but the file name is technically wrong now 😄

[rezaansyed]

I noticed that renaming will bring about changes to a bunch of new files. For that reason, let's just do that as part of another PR? Given how big this one is.

[paulomarg]

Yeah no problem